Add some TODOs

Netflix · Jan 18, 2025 · b24c27e · b24c27e
1 parent 25cee6f
commit b24c27e
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 32 deletions.
diff --git a/hollow/src/main/java/com/netflix/hollow/api/producer/HollowProducer.java b/hollow/src/main/java/com/netflix/hollow/api/producer/HollowProducer.java
@@ -869,7 +869,7 @@ public B withFocusHoleFillInFewestShards(boolean focusHoleFillInFewestShards) {
          * delta transitions with a memory overhead (equal to the configured max shard size).
          *
          * Requires integrity check to be enabled, and honors numShards pinned using annotation in data model.
-         * Also requires consumers to be on a recent Hollow library version that supports re-sharding at the time of delta application.
+         * Also requires consumers of the delta chain to be on a recent Hollow library version that supports re-sharding at the time of delta application.
          */
         public B withTypeResharding(boolean allowTypeResharding) {
             this.allowTypeResharding = allowTypeResharding;

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/HollowObjectTypeWriteState.java b/hollow/src/main/java/com/netflix/hollow/core/write/HollowObjectTypeWriteState.java
@@ -16,7 +16,6 @@
  */
 package com.netflix.hollow.core.write;
 
-import com.netflix.hollow.core.HollowStateEngine;
 import com.netflix.hollow.core.memory.ByteData;
 import com.netflix.hollow.core.memory.ByteDataArray;
 import com.netflix.hollow.core.memory.ThreadSafeBitSet;
@@ -93,7 +92,7 @@ public void prepareForWrite() {
             revNumShards = numShards;
         } else {
             revNumShards = numShards;
-            if (allowTypeResharding()) {
+            if (allowTypeResharding()) {    // SNAP: TODO: extend to other types
                 numShards = targetNumShards(maxOrdinal);
                 if (numShards != revNumShards) {    // re-sharding
                     // limit numShards to 2x or .5x of prevShards per producer cycle
@@ -122,20 +121,6 @@ private int targetNumShards(int maxOrdinal) {
         return targetNumShards;
     }
 
-    /**
-     * A header tag indicating that num shards for a type has changed since the prior version. Its value encodes
-     * the type(s) that were re-sharded along with the before and after num shards in the fwd delta direction.
-     * For e.g. Movie:(2,4) Actor:(8,4)
-     */
-    private void addReshardingHeader(int prevNumShards, int newNumShards) {
-        String existing = stateEngine.getHeaderTag(HollowStateEngine.HEADER_TAG_TYPE_RESHARDING_INVOKED);
-        String appendTo = "";
-        if (existing != null) {
-            appendTo = existing + " ";
-        }
-        stateEngine.addHeaderTag(HollowStateEngine.HEADER_TAG_TYPE_RESHARDING_INVOKED, appendTo + schema.getName() + ":(" + prevNumShards + "," + newNumShards + ")");
-    }
-
     int[] calcMaxShardOrdinal(int maxOrdinal, int numShards) {
         int[] maxShardOrdinal = new int[numShards];
         int minRecordLocationsPerShard = (maxOrdinal + 1) / numShards;
@@ -298,13 +283,13 @@ public void writeDelta(DataOutputStream dos) throws IOException {
 
     @Override
     public void calculateReverseDelta() {
-        calculateDelta(currentCyclePopulated, previousCyclePopulated, revNumShards);
+        calculateDelta(currentCyclePopulated, previousCyclePopulated, revNumShards);    // SNAP: TODO: extend passing of revNumShards for other types
     }
 
     @Override
     public void writeReverseDelta(DataOutputStream dos) throws IOException {
         LOG.log(Level.FINE, String.format("Writing reversedelta with num shards = %s, max shard ordinals = %s", revNumShards, Arrays.toString(revMaxShardOrdinal)));
-        writeCalculatedDelta(dos, revNumShards, revMaxShardOrdinal);
+        writeCalculatedDelta(dos, revNumShards, revMaxShardOrdinal);    // SNAP: TODO: extend passing of revNumShards and revMaxShardOrdinal for other types
     }
 
     private void calculateDelta(ThreadSafeBitSet fromCyclePopulated, ThreadSafeBitSet toCyclePopulated, int numShards) {

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/HollowTypeWriteState.java b/hollow/src/main/java/com/netflix/hollow/core/write/HollowTypeWriteState.java
@@ -19,6 +19,7 @@
 import static com.netflix.hollow.core.write.HollowHashableWriteRecord.HashBehavior.IGNORED_HASHES;
 import static com.netflix.hollow.core.write.HollowHashableWriteRecord.HashBehavior.UNMIXED_HASHES;
 
+import com.netflix.hollow.core.HollowStateEngine;
 import com.netflix.hollow.core.memory.ByteArrayOrdinalMap;
 import com.netflix.hollow.core.memory.ByteDataArray;
 import com.netflix.hollow.core.memory.ThreadSafeBitSet;
@@ -392,17 +393,27 @@ public HollowWriteStateEngine getStateEngine() {
     }
 
     public boolean allowTypeResharding() {
-        if (this instanceof HollowObjectTypeWriteState) {
-            if (stateEngine.allowTypeResharding()) {
-                if (isNumShardsPinned()) {
-                    LOG.warning("Type re-sharding feature was enabled but num shards is pinned (likely using the " +
-                            "HollowShardLargeType annotation in the data model). Proceeding with fixed num shards.");
-                    return false;
-                }
+        if (stateEngine.allowTypeResharding()) {
+            if (isNumShardsPinned()) {
+                LOG.info("Types will not re-shard automatically because num shards is pinned (likely using the " +
+                        "HollowShardLargeType annotation in the data model). Proceeding with fixed num shards.");
+                return false;
             }
-            return stateEngine.allowTypeResharding();
-        } else {
-            return false;   // only supported for object types
         }
+        return stateEngine.allowTypeResharding();
+    }
+
+    /**
+     * A header tag indicating that num shards for a type has changed since the prior version. Its value encodes
+     * the type(s) that were re-sharded along with the before and after num shards in the fwd delta direction.
+     * For e.g. Movie:(2,4) Actor:(8,4)
+     */
+    protected void addReshardingHeader(int prevNumShards, int newNumShards) {
+        String existing = stateEngine.getHeaderTag(HollowStateEngine.HEADER_TAG_TYPE_RESHARDING_INVOKED);
+        String appendTo = "";
+        if (existing != null) {
+            appendTo = existing + " ";
+        }
+        stateEngine.addHeaderTag(HollowStateEngine.HEADER_TAG_TYPE_RESHARDING_INVOKED, appendTo + schema.getName() + ":(" + prevNumShards + "," + newNumShards + ")");
     }
 }
diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/HollowWriteStateEngine.java b/hollow/src/main/java/com/netflix/hollow/core/write/HollowWriteStateEngine.java
@@ -439,7 +439,7 @@ long getTargetMaxTypeShardSize() {
      * delta transitions with a memory overhead (equal to the configured max shard size).
      *
      * Requires integrity check to be enabled, and honors numShards pinned using annotation in data model.
-     * Also requires consumers to be on a recent Hollow library version that supports re-sharding at the time of delta application.
+     * Also requires all consumers of the delta chain to be on a recent Hollow library version that supports re-sharding at the time of delta application.
      */
     public void allowTypeResharding(boolean allowTypeResharding) {
         this.allowTypeResharding = allowTypeResharding;

diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java
@@ -137,7 +137,7 @@ public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, S
             this.writeState = existingWriteState;
         } else {
             int numShardsByAnnotation = getNumShardsByAnnotation(clazz);
-            this.writeState = new HollowObjectTypeWriteState(schema, numShardsByAnnotation, numShardsByAnnotation == -1 ? false : true);
+            this.writeState = new HollowObjectTypeWriteState(schema, numShardsByAnnotation, numShardsByAnnotation == -1 ? false : true);    // SNAP: TODO: extend to list set etc.
         }
 
         this.assignedOrdinalFieldOffset = assignedOrdinalFieldOffset;
@@ -153,7 +153,7 @@ private static String[] getKeyFieldPaths(Class<?> clazz) {
         return primaryKey == null ? null : primaryKey.fields();
     }
 
-    private static int getNumShardsByAnnotation(Class<?> clazz) {
+    private static int getNumShardsByAnnotation(Class<?> clazz) {   // SNAP: TODO: can be on non-object types like https://stash.corp.netflix.com/users/sunjeets/repos/hollow/browse/hollow/src/test/java/com/netflix/hollow/api/consumer/FocusedShardHoleFillTest.java#190-191
         HollowShardLargeType numShardsAnnotation = clazz.getAnnotation(HollowShardLargeType.class);
         if(numShardsAnnotation != null)
             return numShardsAnnotation.numShards();