Jackie-Jiang commented on code in PR #13449:
URL: https://github.com/apache/pinot/pull/13449#discussion_r1653266496
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -806,11 +822,8 @@ protected void doRemoveSegment(IndexSegment segment) {
long startTimeMs = System.currentTimeMillis();
MutableRoaringBitmap validDocIds =
- segment.getValidDocIds() != null ?
segment.getValidDocIds().getMutableRoaringBitmap() : null;
- if (validDocIds == null || validDocIds.isEmpty()) {
- _logger.info("Skip removing segment without valid docs: {}",
segmentName);
- return;
- }
+ segment.getValidDocIds() != null ?
segment.getValidDocIds().getMutableRoaringBitmap()
+ : new MutableRoaringBitmap();
Review Comment:
Can you elaborate more on this:
> What I meant is in the base class, early termination might be an issue
since in another extension, we might want to iterate through the segment and
not just valid doc ids.
Trying to see what functionality you are seeking for.
Even after moving the early termination to `removeSegment()`, there will be
extra overhead following `removeSegment()` and logging changes. More
importantly, the current contract for `removeSegment()` is that the segment
contains valid docs (it is also invoked when replacing segment). In order to
handle segment removal differently, you can override `doRemoveSegment()` instead
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
