rohityadav1993 commented on code in PR #17352:
URL: https://github.com/apache/pinot/pull/17352#discussion_r2619900392
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java:
##########
@@ -286,7 +292,17 @@ public static SegmentSelectionResult
processValidDocIdsMetadata(String tableName
continue;
}
SegmentZKMetadata segment = candidateSegmentsMap.get(segmentName);
- for (ValidDocIdsMetadataInfo validDocIdsMetadata :
validDocIdsMetadataInfoMap.get(segmentName)) {
+ List<ValidDocIdsMetadataInfo> replicaMetadataList =
validDocIdsMetadataInfoMap.get(segmentName);
+
+ // Check consensus across all replicas before proceeding with any
operations
+ if (!hasValidDocConsensus(segmentName, replicaMetadataList)) {
Review Comment:
This will reduce the chances but is it still possible that a subsequent
restart/repalce and doSnapshot leads to a segment with 0 validDocIds to become
non zero again?
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java:
##########
@@ -298,20 +314,11 @@ public static SegmentSelectionResult
processValidDocIdsMetadata(String tableName
continue;
}
- // skipping segments for which their servers are not in READY state.
The bitmaps would be inconsistent when
- // server is NOT READY as UPDATING segments might be updating the
ONLINE segments
- if (validDocIdsMetadata.getServerStatus() != null &&
!validDocIdsMetadata.getServerStatus()
- .equals(ServiceStatus.Status.GOOD)) {
- LOGGER.warn("Server {} is in {} state, skipping {} generation for
segment: {}",
- validDocIdsMetadata.getInstanceId(),
validDocIdsMetadata.getServerStatus(),
- MinionConstants.UpsertCompactMergeTask.TASK_TYPE, segmentName);
- continue;
- }
-
// segments eligible for deletion with no valid records
long totalDocs = validDocIdsMetadata.getTotalDocs();
if (totalInvalidDocs == totalDocs) {
Review Comment:
can we add a metric, this should help us give an idea if there is too much
deviation in replicas
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java:
##########
@@ -298,20 +314,11 @@ public static SegmentSelectionResult
processValidDocIdsMetadata(String tableName
continue;
}
- // skipping segments for which their servers are not in READY state.
The bitmaps would be inconsistent when
- // server is NOT READY as UPDATING segments might be updating the
ONLINE segments
- if (validDocIdsMetadata.getServerStatus() != null &&
!validDocIdsMetadata.getServerStatus()
- .equals(ServiceStatus.Status.GOOD)) {
- LOGGER.warn("Server {} is in {} state, skipping {} generation for
segment: {}",
- validDocIdsMetadata.getInstanceId(),
validDocIdsMetadata.getServerStatus(),
- MinionConstants.UpsertCompactMergeTask.TASK_TYPE, segmentName);
- continue;
- }
-
// segments eligible for deletion with no valid records
long totalDocs = validDocIdsMetadata.getTotalDocs();
if (totalInvalidDocs == totalDocs) {
segmentsForDeletion.add(segmentName);
+ LOGGER.info("Segment {} marked for deletion - all replicas agree it
has zero valid documents", segmentName);
Review Comment:
> all replicas agree it has zero valid documents"
nit: since we already validated replica earlier this is a redundant log
message
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java:
##########
@@ -410,6 +417,48 @@ public static SegmentSelectionResult
processValidDocIdsMetadata(String tableName
return new SegmentSelectionResult(groupedSegments, new
ArrayList<>(segmentsForDeletion));
}
+ /**
+ * Checks if all replicas have consensus on validDoc counts for a segment.
+ * SAFETY LOGIC:
+ * 1. Only proceed with operations when ALL replicas agree on totalValidDocs
count
+ * 2. Skip operations if ANY server hosting the segment is not in READY state
+ * 3. Include all replicas (even those with CRC mismatches) in consensus for
deletion safety
+ */
+ @VisibleForTesting
+ public static boolean hasValidDocConsensus(String segmentName,
Review Comment:
This logic will be good to reuse and can be moved to a util class.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
