J-HowHuang commented on code in PR #16096:
URL: https://github.com/apache/pinot/pull/16096#discussion_r2155290463
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2185,7 +2185,7 @@ private void processBatchesSequentially(List<Set<String>>
segmentBatchList, Stri
}
}
- private void waitUntilPrevBatchIsComplete(String tableNameWithType,
Set<String> segmentBatchToCommit,
+ public void waitUntilPrevBatchIsComplete(String tableNameWithType,
Set<String> segmentBatchToCommit,
Review Comment:
https://github.com/apache/pinot/blob/f81b03c20493c9dd8ed818c1706262eb2dd64797/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java#L255-L263
Also the controller API uses the method `getSegmentsYetToBeCommitted` too to
track the forceCommit status, which is why I think it's okay to use it in
https://github.com/apache/pinot/commit/b912b6a47af5a72f9c11fc9ef62ae0d67b8fad52
(the duplicate code is of <10 lines, which is only a general check and retry
and I think it's acceptable to be duplicated).
Given the same reason (external use of method in
`PinotLLCRealtimeSegmentManager`) I think it's also acceptable to make
`waitUntilPrevBatchIsComplete` public, as my original approach. Perhaps with a
change of its name to `waitUntilSegmentsAreComplete` to make the intent generic.
I prefer not to make an util for this, as I don't think this will be common
enough to be so.
@somandal What's your opinion on this?
--
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]
