[GitHub] [pinot] yupeng9 commented on a diff in pull request #9062: [Upsert] persist validDocsIndex snapshot for Pinot upsert optimization

[GitBox <gi...@apache.org>]

yupeng9 commented on code in PR #9062:
URL: https://github.com/apache/pinot/pull/9062#discussion_r929396638


##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -348,7 +348,7 @@ protected Map<String, String> getStreamConfigMap() {
       streamConfigMap.put(KafkaStreamConfigProperties.constructStreamProperty(
           KafkaStreamConfigProperties.HighLevelConsumer.KAFKA_HLC_ZK_CONNECTION_STRING), getKafkaZKAddress());
       streamConfigMap.put(KafkaStreamConfigProperties.constructStreamProperty(
-              KafkaStreamConfigProperties.HighLevelConsumer.KAFKA_HLC_BOOTSTRAP_SERVER),
+          KafkaStreamConfigProperties.HighLevelConsumer.KAFKA_HLC_BOOTSTRAP_SERVER),

Review Comment:
   curious, do you use the right formatter?



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -213,6 +214,13 @@ public List<StarTreeV2> getStarTrees() {
     return _starTreeIndexContainer != null ? _starTreeIndexContainer.getStarTrees() : null;
   }
 
+  @Nullable
+  public void setValidDocIds(ImmutableRoaringBitmap validDocIds) {
+    for (int docId: validDocIds.toArray()) {
+      _validDocIds.add(docId);

Review Comment:
   this is not set, but add?



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java:
##########
@@ -142,7 +142,7 @@ public class OfflineClusterIntegrationTest extends BaseClusterIntegrationTestSet
   private static final String MAX_NUM_MULTI_VALUES_MAP_KEY = "maxNumMultiValuesMap";
   // TODO: This might lead to flaky test, as this disk size is not deterministic
   //       as it depends on the iteration order of a HashSet.
-  private static final int DISK_SIZE_IN_BYTES = 20796000;
+  private static final int DISK_SIZE_IN_BYTES = 20796348;

Review Comment:
   why change on this?



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java:
##########
@@ -198,6 +201,13 @@ public void init(SegmentGeneratorConfig config, SegmentCreationDataSource dataSo
     // Create a temporary directory used in segment creation
     _tempIndexDir = new File(indexDir, "tmp-" + UUID.randomUUID());
     LOGGER.debug("tempIndexDir:{}", _tempIndexDir);
+
+    _upsertSnapshotEnabled = _config.isUpsertSnapshotEnabled();

Review Comment:
   Should we do this in `build` method? I think this shall be enabled with TTL, as it applies only to the segments outside TTL, otherwise the valid doc id will keep changing



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org