[GitHub] [pinot] mcvsubbu commented on a change in pull request #8035: refine segment consistency check

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

mcvsubbu commented on a change in pull request #8035:
URL: https://github.com/apache/pinot/pull/8035#discussion_r787170735



##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/JsonIndexHandler.java
##########
@@ -59,8 +59,24 @@ public JsonIndexHandler(SegmentMetadata segmentMetadata, IndexLoadingConfig inde
 
   @Override
   public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
+    String segmentName = _segmentMetadata.getName();
     Set<String> existingColumns = segmentReader.toSegmentDirectory().getColumnsWithIndex(ColumnIndexType.JSON_INDEX);
-    return !existingColumns.equals(_columnsToAddIdx);
+    // Check if any existing index need to be removed.
+    for (String column : existingColumns) {
+      if (!_columnsToAddIdx.remove(column)) {
+        LOGGER.debug("Need to remove existing json index from segment: {}, column: {}", segmentName, column);

Review comment:
       same here, info will help

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/RangeIndexHandler.java
##########
@@ -58,8 +58,25 @@ public RangeIndexHandler(SegmentMetadata segmentMetadata, IndexLoadingConfig ind
 
   @Override
   public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
+    String segmentName = _segmentMetadata.getName();
     Set<String> existingColumns = segmentReader.toSegmentDirectory().getColumnsWithIndex(ColumnIndexType.RANGE_INDEX);
-    return !existingColumns.equals(_columnsToAddIdx);
+    // Check if any existing index need to be removed.
+    for (String column : existingColumns) {
+      if (!_columnsToAddIdx.remove(column)) {
+        LOGGER.debug("Need to remove existing range index from segment: {}, column: {}", segmentName, column);

Review comment:
       totally agreed. More than one log at load time does not hurt. The more verbose we are, the easlier it is to locate problems

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/JsonIndexHandler.java
##########
@@ -59,8 +59,24 @@ public JsonIndexHandler(SegmentMetadata segmentMetadata, IndexLoadingConfig inde
 
   @Override
   public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
+    String segmentName = _segmentMetadata.getName();
     Set<String> existingColumns = segmentReader.toSegmentDirectory().getColumnsWithIndex(ColumnIndexType.JSON_INDEX);
-    return !existingColumns.equals(_columnsToAddIdx);
+    // Check if any existing index need to be removed.
+    for (String column : existingColumns) {
+      if (!_columnsToAddIdx.remove(column)) {
+        LOGGER.debug("Need to remove existing json index from segment: {}, column: {}", segmentName, column);
+        return true;
+      }
+    }
+    // Check if any new index need to be added.
+    for (String column : _columnsToAddIdx) {
+      ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(column);
+      if (columnMetadata != null) {
+        LOGGER.debug("Need to create new json index for segment: {}, column: {}", segmentName, column);

Review comment:
       ditto

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java
##########
@@ -143,22 +143,26 @@ public boolean needProcess()
         DefaultColumnHandler defaultColumnHandler = DefaultColumnHandlerFactory
             .getDefaultColumnHandler(null, _segmentMetadata, _indexLoadingConfig, _schema, null);
         if (defaultColumnHandler.needUpdateDefaultColumns()) {
+          LOGGER.debug("Found default columns need updates");
           return true;
         }
       }
       // Check if there is need to update single-column indices, like inverted index, json index etc.
       for (ColumnIndexType type : ColumnIndexType.values()) {
         if (IndexHandlerFactory.getIndexHandler(type, _segmentMetadata, _indexLoadingConfig)
             .needUpdateIndices(segmentReader)) {
+          LOGGER.debug("Found index type: {} needs updates", type);
           return true;
         }
       }
       // Check if there is need to create/modify/remove star-trees.
       if (needProcessStarTrees()) {
+        LOGGER.debug("Found startree index needs updates");
         return true;
       }
       // Check if there is need to update column min max value.
       if (needUpdateColumnMinMaxValue()) {

Review comment:
       +1




-- 
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