[GitHub] [druid] imply-cheddar commented on a diff in pull request #13070: Allocate numCorePartitions using only used segments

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

imply-cheddar commented on code in PR #13070:
URL: https://github.com/apache/druid/pull/13070#discussion_r971469785


##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -848,6 +848,11 @@ private SegmentIdWithShardSpec createNewSegment(
         versionOfExistingChunk = null;
       }
 
+      // The number of core partitions must always be chosen from the set of used segments in the VersionedIntervalTimeline.
+      // When the core partitions have been dropped, using pending segments may lead to an incorrect state
+      // where the chunk is believed to have core partitions and queries results are incorrect.
+      int numCorePartitions = maxId == null ? 0 : maxId.getShardSpec().getNumCorePartitions();

Review Comment:
   The change as written requires an understanding that the `maxId` variable is going to be replaced to understand that this specific position of this line is very important for correctness.  I might recommend that we maintain a `commitedMaxId` and a `overallMaxId` as two separate variables and then use the correct one down at the bottom.  This makes it more clear what the state that we are depending on is.



-- 
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@druid.apache.org

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


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