From 7ae0d27b54a0bce566caa8d48f45aa1f1d900ed0 Mon Sep 17 00:00:00 2001 From: Sunjeet Singh Date: Tue, 2 Apr 2024 07:53:47 -0700 Subject: [PATCH] Primary and hash key index warn about non-bindable field/type at init time and fail at query time --- .../netflix/hollow/core/index/FieldPaths.java | 9 ++- .../hollow/core/index/HollowHashIndex.java | 45 +++++++++++++- .../core/index/HollowPrimaryKeyIndex.java | 60 +++++++++++++++++-- .../core/index/HollowHashIndexTest.java | 48 ++++++++++++++- .../core/index/HollowPrimaryKeyIndexTest.java | 43 +++++++++++++ 5 files changed, 193 insertions(+), 12 deletions(-) diff --git a/hollow/src/main/java/com/netflix/hollow/core/index/FieldPaths.java b/hollow/src/main/java/com/netflix/hollow/core/index/FieldPaths.java index a6b5cb1a46..8d49e2b897 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/index/FieldPaths.java +++ b/hollow/src/main/java/com/netflix/hollow/core/index/FieldPaths.java @@ -29,12 +29,16 @@ import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.logging.Level; +import java.util.logging.Logger; /** * Functionality for processing field paths. */ public final class FieldPaths { + private static final Logger LOG = Logger.getLogger(FieldPaths.class.getName()); + /** * Creates an object-based field path given a data set and the field path in symbolic form conforming to paths * associated with a primary key. @@ -124,8 +128,9 @@ static FieldPath createFieldPath( HollowSchema schema = dataset.getSchema(segmentType); // @@@ Can this only occur for anything other than the root `type`? if (schema == null) { - throw new FieldPathException(FieldPathException.ErrorKind.NOT_BINDABLE, dataset, type, segments, - fieldSegments, null, i); + LOG.log(Level.WARNING, FieldPathException.message(FieldPathException.ErrorKind.NOT_BINDABLE, dataset, + type, segments, fieldSegments, null, i)); + throw new FieldPathException(FieldPathException.ErrorKind.NOT_BINDABLE, dataset, type, segments, fieldSegments, null, i); } String segment = segments[i]; diff --git a/hollow/src/main/java/com/netflix/hollow/core/index/HollowHashIndex.java b/hollow/src/main/java/com/netflix/hollow/core/index/HollowHashIndex.java index 94bcf5d838..df72d5188d 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/index/HollowHashIndex.java +++ b/hollow/src/main/java/com/netflix/hollow/core/index/HollowHashIndex.java @@ -16,6 +16,7 @@ */ package com.netflix.hollow.core.index; +import static com.netflix.hollow.core.index.FieldPaths.FieldPathException.ErrorKind.NOT_BINDABLE; import static java.util.Objects.requireNonNull; import com.netflix.hollow.core.HollowConstants; @@ -28,6 +29,9 @@ import com.netflix.hollow.core.read.engine.HollowReadStateEngine; import com.netflix.hollow.core.read.engine.HollowTypeStateListener; import com.netflix.hollow.core.read.engine.object.HollowObjectTypeReadState; +import java.util.Arrays; +import java.util.logging.Level; +import java.util.logging.Logger; /** * A HollowHashIndex is used for indexing non-primary-key data. This type of index can map multiple keys to a single matching record, and/or @@ -38,6 +42,7 @@ * actors, each elements contained therein, and finally each actors actorId field. */ public class HollowHashIndex implements HollowTypeStateListener { + private static final Logger LOG = Logger.getLogger(HollowHashIndex.class.getName()); private volatile HollowHashIndexState hashStateVolatile; @@ -81,6 +86,11 @@ public HollowHashIndex(HollowDataAccess hollowDataAccess, String type, String se this.selectField = selectField; this.matchFields = matchFields; + if (typeState == null) { + LOG.log(Level.WARNING, "Index initialization for " + this + " failed because type " + + type + " was not found in read state"); + return; + } reindexHashIndex(); } @@ -88,10 +98,22 @@ public HollowHashIndex(HollowDataAccess hollowDataAccess, String type, String se * Recreate the hash index entirely */ private void reindexHashIndex() { - HollowHashIndexBuilder builder = new HollowHashIndexBuilder(hollowDataAccess, type, selectField, matchFields); + HollowHashIndexBuilder builder; + try { + builder = new HollowHashIndexBuilder(hollowDataAccess, type, selectField, matchFields); + } catch (FieldPaths.FieldPathException e) { + if (e.error == NOT_BINDABLE) { + LOG.log(Level.WARNING, "Index initialization for " + this + + " failed because one of the match fields could not be bound to a type in" + + " the read state"); + this.hashStateVolatile = null; + return; + } else { + throw e; + } + } builder.buildIndex(); - this.hashStateVolatile = new HollowHashIndexState(builder); } @@ -104,6 +126,9 @@ private void reindexHashIndex() { * found. */ public HollowHashIndexResult findMatches(Object... query) { + if (hashStateVolatile == null) { + throw new IllegalStateException(this + " wasn't initialized"); + } int hashCode = 0; for(int i=0;i * In order to prevent memory leaks, if this method is called and the index is no longer needed, call detachFromDeltaUpdates() before * discarding the index. + *

+ * Note that this index does not listen on snapshot update. If a snapshot update occurs this index will + * NOT return the latest data in the consumer. Callers must detach this index instance and initialize a new + * one on snapshot update, or disable double snapshot update for the consumer altogether. */ public void listenForDeltaUpdates() { + if (typeState == null) { + return; + } if(specificOrdinalsToIndex != null) throw new IllegalStateException("Cannot listen for delta updates when indexing only specified ordinals!"); @@ -138,6 +163,9 @@ public void listenForDeltaUpdates() { * Call this method before discarding indexes which are currently listening for delta updates. */ public void detachFromDeltaUpdates() { + if (typeState == null) { + return; + } typeState.removeListener(this); } @@ -162,6 +190,10 @@ public List getFieldTypes() { * @return the matching ordinal for the key, otherwise -1 if the key is not present */ public int getMatchingOrdinal(Object key) { + if (hashTableVolatile == null) { + throw new IllegalStateException("Index " + primaryKey.toString() + " wasn't initialized"); + } + PrimaryKeyIndexHashTable hashTable = hashTableVolatile; if(fieldPathIndexes.length != 1 || hashTable.bitsPerElement == 0) return -1; @@ -197,6 +229,10 @@ public int getMatchingOrdinal(Object key) { * @return the matching ordinal for the two keys, otherwise -1 if the key is not present */ public int getMatchingOrdinal(Object key1, Object key2) { + if (hashTableVolatile == null) { + throw new IllegalStateException("Index " + primaryKey.toString() + " wasn't initialized"); + } + PrimaryKeyIndexHashTable hashTable = hashTableVolatile; if(fieldPathIndexes.length != 2 || hashTable.bitsPerElement == 0) return -1; @@ -234,6 +270,10 @@ public int getMatchingOrdinal(Object key1, Object key2) { * @return the matching ordinal for the three keys, otherwise -1 if the key is not present */ public int getMatchingOrdinal(Object key1, Object key2, Object key3) { + if (hashTableVolatile == null) { + throw new IllegalStateException("Index " + primaryKey.toString() + " wasn't initialized"); + } + PrimaryKeyIndexHashTable hashTable = hashTableVolatile; if(fieldPathIndexes.length != 3 || hashTable.bitsPerElement == 0) return -1; @@ -270,6 +310,10 @@ public int getMatchingOrdinal(Object key1, Object key2, Object key3) { * @return the matching ordinal for the keys, otherwise -1 if the key is not present */ public int getMatchingOrdinal(Object... keys) { + if (hashTableVolatile == null) { + throw new IllegalStateException("Index " + primaryKey.toString() + " wasn't initialized"); + } + PrimaryKeyIndexHashTable hashTable = hashTableVolatile; if(fieldPathIndexes.length != keys.length || hashTable.bitsPerElement == 0) return -1; @@ -378,6 +422,10 @@ public void removedOrdinal(int ordinal) { } @Override public synchronized void endUpdate() { + if (hashTableVolatile == null) { + return; + } + BitSet ordinals = typeState.getListener(PopulatedOrdinalListener.class).getPopulatedOrdinals(); int hashTableSize = HashCodes.hashTableSize(ordinals.cardinality()); diff --git a/hollow/src/test/java/com/netflix/hollow/core/index/HollowHashIndexTest.java b/hollow/src/test/java/com/netflix/hollow/core/index/HollowHashIndexTest.java index c61e965f3b..3bc1185efa 100644 --- a/hollow/src/test/java/com/netflix/hollow/core/index/HollowHashIndexTest.java +++ b/hollow/src/test/java/com/netflix/hollow/core/index/HollowHashIndexTest.java @@ -19,11 +19,19 @@ import static java.util.Arrays.stream; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; +import static org.junit.Assert.fail; import com.netflix.hollow.core.AbstractStateEngineTest; +import com.netflix.hollow.core.read.engine.HollowReadStateEngine; import com.netflix.hollow.core.read.iterator.HollowOrdinalIterator; +import com.netflix.hollow.core.schema.HollowObjectSchema; +import com.netflix.hollow.core.util.StateEngineRoundTripper; +import com.netflix.hollow.core.write.HollowObjectTypeWriteState; +import com.netflix.hollow.core.write.HollowObjectWriteRecord; +import com.netflix.hollow.core.write.HollowWriteStateEngine; import com.netflix.hollow.core.write.objectmapper.HollowInline; import com.netflix.hollow.core.write.objectmapper.HollowObjectMapper; +import java.io.IOException; import java.util.Arrays; import java.util.HashSet; import java.util.List; @@ -261,7 +269,7 @@ public void testFindingMatchForNullQueryValue() throws Exception { roundTripSnapshot(); HollowHashIndex index = new HollowHashIndex(readStateEngine, "TypeB", "", "b1.value"); index.findMatches(new Object[]{null}); - Assert.fail("exception expected"); + fail("exception expected"); } @Test @@ -321,6 +329,44 @@ public void testGettingPropertiesValues() throws Exception { Assert.assertEquals(index.getSelectField(), ""); } + @Test + public void testNotBindable() throws IOException { + HollowWriteStateEngine writeEngine = new HollowWriteStateEngine(); + HollowObjectSchema movieSchema = new HollowObjectSchema("Movie", 3); + movieSchema.addField("id", HollowObjectSchema.FieldType.LONG); + movieSchema.addField("title", HollowObjectSchema.FieldType.REFERENCE, "String"); + movieSchema.addField("releaseYear", HollowObjectSchema.FieldType.INT); + HollowObjectTypeWriteState movieState = new HollowObjectTypeWriteState(movieSchema); + writeEngine.addTypeState(movieState); + + HollowObjectWriteRecord movieRec = new HollowObjectWriteRecord(movieSchema); + movieRec.setLong("id", 1); + movieRec.setReference("title", 0); // NOTE that String type wasn't added + movieRec.setInt("releaseYear", 1999); + writeEngine.add("Movie", movieRec); + + HollowReadStateEngine readEngine = new HollowReadStateEngine(); + StateEngineRoundTripper.roundTripSnapshot(writeEngine, readEngine); + + // invalid because root type doesn't exist + HollowHashIndex invalidHashIndex1 = new HollowHashIndex(readEngine, "String", "value", "value"); + try { + invalidHashIndex1.findMatches("test"); + fail("Index on root type not bound is expected to fail hard at query time"); + } catch (IllegalStateException e) {} + + // invalid because a type in the field paths doesn't exist + HollowHashIndex invalidHashIndex2 = new HollowHashIndex(readEngine, "Movie", "id", "title.value"); + try { + invalidHashIndex2.findMatches("test"); + fail("Index on field path not bound is expected to fail hard at query time"); + } catch (IllegalStateException e) {} + + // valid index despite a non-indexed field (title) not bindable to a type (String) + HollowPrimaryKeyIndex validPki = new HollowPrimaryKeyIndex(readEngine, "Movie", "id"); + Assert.assertEquals(0, validPki.getMatchingOrdinal(1L)); + } + private void assertIteratorContainsAll(HollowOrdinalIterator iter, int... expectedOrdinals) { Set ordinalSet = new HashSet<>(); int ordinal = iter.next(); diff --git a/hollow/src/test/java/com/netflix/hollow/core/index/HollowPrimaryKeyIndexTest.java b/hollow/src/test/java/com/netflix/hollow/core/index/HollowPrimaryKeyIndexTest.java index 61300164b8..c4ecee6a62 100644 --- a/hollow/src/test/java/com/netflix/hollow/core/index/HollowPrimaryKeyIndexTest.java +++ b/hollow/src/test/java/com/netflix/hollow/core/index/HollowPrimaryKeyIndexTest.java @@ -16,12 +16,17 @@ */ package com.netflix.hollow.core.index; +import static org.junit.Assert.fail; + import com.netflix.hollow.core.AbstractStateEngineTest; import com.netflix.hollow.core.memory.pool.ArraySegmentRecycler; +import com.netflix.hollow.core.read.engine.HollowReadStateEngine; import com.netflix.hollow.core.read.engine.object.HollowObjectTypeReadState; import com.netflix.hollow.core.schema.HollowObjectSchema; import com.netflix.hollow.core.schema.HollowObjectSchema.FieldType; +import com.netflix.hollow.core.util.StateEngineRoundTripper; import com.netflix.hollow.core.write.HollowObjectTypeWriteState; +import com.netflix.hollow.core.write.HollowObjectWriteRecord; import com.netflix.hollow.core.write.HollowWriteStateEngine; import com.netflix.hollow.core.write.objectmapper.HollowObjectMapper; import com.netflix.hollow.core.write.objectmapper.HollowPrimaryKey; @@ -292,6 +297,44 @@ public void testDups() throws IOException { } } + @Test + public void testNotBindable() throws IOException { + HollowWriteStateEngine writeEngine = new HollowWriteStateEngine(); + HollowObjectSchema movieSchema = new HollowObjectSchema("Movie", 3); + movieSchema.addField("id", HollowObjectSchema.FieldType.LONG); + movieSchema.addField("title", HollowObjectSchema.FieldType.REFERENCE, "String"); + movieSchema.addField("releaseYear", HollowObjectSchema.FieldType.INT); + HollowObjectTypeWriteState movieState = new HollowObjectTypeWriteState(movieSchema); + writeEngine.addTypeState(movieState); + + HollowObjectWriteRecord movieRec = new HollowObjectWriteRecord(movieSchema); + movieRec.setLong("id", 1); + movieRec.setReference("title", 0); // NOTE that String type wasn't added + movieRec.setInt("releaseYear", 1999); + writeEngine.add("Movie", movieRec); + + HollowReadStateEngine readEngine = new HollowReadStateEngine(); + StateEngineRoundTripper.roundTripSnapshot(writeEngine, readEngine); + + // invalid because root type doesn't exist + HollowPrimaryKeyIndex invalidPki1 = new HollowPrimaryKeyIndex(readEngine, "String", "value"); + try { + invalidPki1.getMatchingOrdinal("test"); + fail("Index on root type not bound is expected to fail hard at query time"); + } catch (IllegalStateException e) {} + + // invalid because a type in the field paths doesn't exist + HollowPrimaryKeyIndex invalidPki2 = new HollowPrimaryKeyIndex(readEngine, "Movie", "title.value"); + try { + invalidPki2.getMatchingOrdinal(1L); + fail("Index on field path not bound is expected to fail hard at query time"); + } catch (IllegalStateException e) {} + + // valid index despite a non-indexed field (title) not bindable to a type (String) + HollowPrimaryKeyIndex validPki = new HollowPrimaryKeyIndex(readEngine, "Movie", "id"); + Assert.assertEquals(0, validPki.getMatchingOrdinal(1L)); + } + private static void addDataForDupTesting(HollowWriteStateEngine writeStateEngine, int a1Start, double a2, int size) { TypeB typeB = new TypeB("commonTypeB"); HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine);