You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "deepthi912 (via GitHub)" <gi...@apache.org> on 2026/03/29 22:37:29 UTC
[PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
deepthi912 opened a new pull request, #18020:
URL: https://github.com/apache/pinot/pull/18020
- Replaced segments and removed segments are not cleaned up from tracking set (`_updatedSegmentsSinceLastSnapshot`): Pinot tracks which segments need their state saved to disk. When a segment gets replaced (e.g. during a reload), the old segment is removed from the active list but not from the tracking set. It stays there until the next periodic save which only happens every 3-20 hours. Each partition leaks ~248 MB this way.
- Cloning the bitmap while persisting snapshot of all the segments that are updated would bring up the heap usage to pretty high value if the segment commits happen frequently. Instead of doing so, synchronized the serialization process of existing `ThreadSafeMutableRoaringBitmap` to bring down the number of objects created by this 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: 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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3013792572
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -160,13 +159,14 @@ public void persistDocIdsSnapshot(String fileName, ThreadSafeMutableRoaringBitma
LOGGER.warn("Previous snapshot was not taken cleanly. Remove tmp file: {}", tmpFile);
FileUtils.deleteQuietly(tmpFile);
}
- MutableRoaringBitmap docIdsSnapshot = docIds.getMutableRoaringBitmap();
- try (DataOutputStream dataOutputStream = new DataOutputStream(new FileOutputStream(tmpFile))) {
- docIdsSnapshot.serialize(dataOutputStream);
+ byte[] bytes = docIds.toBytes();
Review Comment:
Yeah I haven't seen any longer pause from the small benchmark I have taken . Older code was completely cloning the bitmap. Atleast with that we reduce the number of lock contentions as well as bytes which are eligible for gc immediately. With off-heap memory:
It might not be worth it here:
Direct buffers are slower to allocate. ByteBuffer.allocateDirect() requires a system call (mmap) and has higher per-allocation overhead. For a ~40KB buffer created per segment, this would add overhead dominate any benefit. For maybe larger bitmaps it might make sense but it doesn't explain the purpose. We might just use it to save the heap memory and bring down heap overhead or long gc pauses. I don't think in our scenario we had a long gc pause. @KKcorps correct me if I am wrong.
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #18020:
URL: https://github.com/apache/pinot/pull/18020
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3013367014
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java:
##########
@@ -46,7 +46,6 @@ public LazyRow() {
}
public void init(IndexSegment segment, int docId) {
- clear();
Review Comment:
i feel other folks who have custom pinot forks may still be relying on init to clear. let's just keep it for now
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3010877242
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -962,24 +966,18 @@ protected void doTakeSnapshot() {
continue;
}
ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
- immutableSegment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getValidDocIds();
- immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- validDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap = immutableSegment.getValidDocIds();
Review Comment:
The reason the null check has been removed is because, null bitmap is possible only in 2 cases:
1. When the table is not an upsert table - Then it will not even hit this flow
2. When the segment is out of TTL and has no snapshots taken, file doesn't exist. In such cases, we are not hitting this code path anyway. So only added null check below.
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3016852706
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java:
##########
@@ -46,7 +46,6 @@ public LazyRow() {
}
public void init(IndexSegment segment, int docId) {
- clear();
Review Comment:
yeah, added it back
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3013656036
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -962,24 +966,18 @@ protected void doTakeSnapshot() {
continue;
}
ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
- immutableSegment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getValidDocIds();
- immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- validDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap = immutableSegment.getValidDocIds();
+ immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME, validDocIdsBitMap);
if (_deleteRecordColumn != null) {
- queryableDocIdsBitMap =
- immutableSegment.getQueryableDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getQueryableDocIds();
+ queryableDocIdsBitMap = immutableSegment.getQueryableDocIds();
immutableSegment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
queryableDocIdsBitMap);
}
_updatedSegmentsSinceLastSnapshot.remove(segment);
numImmutableSegments++;
- numPrimaryKeysInSnapshot += validDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
+ numPrimaryKeysInSnapshot += validDocIdsBitMap.getCardinality();
Review Comment:
Good point
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18020:
URL: https://github.com/apache/pinot/pull/18020#issuecomment-4151306379
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18020?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `24 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.49%. Comparing base ([`43c8ecc`](https://app.codecov.io/gh/apache/pinot/commit/43c8ecccffdadf6a2b31b3383f4462ca0f7e2994?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fc699d7`](https://app.codecov.io/gh/apache/pinot/commit/fc699d743615db8baadc6555cb350018835123d2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 52 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18020?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cal/upsert/BasePartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/18020?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2FBasePartitionUpsertMetadataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18020?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...l/indexsegment/immutable/ImmutableSegmentImpl.java](https://app.codecov.io/gh/apache/pinot/pull/18020?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Findexsegment%2Fimmutable%2FImmutableSegmentImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvaW1tdXRhYmxlL0ltbXV0YWJsZVNlZ21lbnRJbXBsLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18020?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../index/mutable/ThreadSafeMutableRoaringBitmap.java](https://app.codecov.io/gh/apache/pinot/pull/18020?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fmutable%2FThreadSafeMutableRoaringBitmap.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L211dGFibGUvVGhyZWFkU2FmZU11dGFibGVSb2FyaW5nQml0bWFwLmphdmE=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18020?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (43c8ecc) and HEAD (fc699d7). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (43c8ecc) | HEAD (fc699d7) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18020 +/- ##
============================================
- Coverage 63.29% 55.49% -7.80%
+ Complexity 1525 745 -780
============================================
Files 3194 2502 -692
Lines 193645 143114 -50531
Branches 29787 22964 -6823
============================================
- Hits 122559 79427 -43132
+ Misses 61466 56936 -4530
+ Partials 9620 6751 -2869
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-7.74%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-7.80%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-7.79%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-0.06%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18020?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3013792572
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -160,13 +159,14 @@ public void persistDocIdsSnapshot(String fileName, ThreadSafeMutableRoaringBitma
LOGGER.warn("Previous snapshot was not taken cleanly. Remove tmp file: {}", tmpFile);
FileUtils.deleteQuietly(tmpFile);
}
- MutableRoaringBitmap docIdsSnapshot = docIds.getMutableRoaringBitmap();
- try (DataOutputStream dataOutputStream = new DataOutputStream(new FileOutputStream(tmpFile))) {
- docIdsSnapshot.serialize(dataOutputStream);
+ byte[] bytes = docIds.toBytes();
Review Comment:
Yeah I haven't seen any longer pause from the small benchmark I have taken . Older code was completely cloning the bitmap entirely. Atleast with that we reduce the number of lock contentions as well as bytes which are eligible for gc immediately. With off-heap memory:
It's might not be worth it here:
Direct buffers are slower to allocate. ByteBuffer.allocateDirect() requires a system call (mmap) and has higher per-allocation overhead. For a ~40KB buffer created per segment, this would add overhead dominate any benefit. For maybe larger bitmaps it might make sense but it doesn't explain the purpose. We might just use it to save the heap memory and bring down heap overhead or long gc pauses. I don't think in our scenario we had a long gc pause. @KKcorps correct me if I am wrong.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -160,13 +159,14 @@ public void persistDocIdsSnapshot(String fileName, ThreadSafeMutableRoaringBitma
LOGGER.warn("Previous snapshot was not taken cleanly. Remove tmp file: {}", tmpFile);
FileUtils.deleteQuietly(tmpFile);
}
- MutableRoaringBitmap docIdsSnapshot = docIds.getMutableRoaringBitmap();
- try (DataOutputStream dataOutputStream = new DataOutputStream(new FileOutputStream(tmpFile))) {
- docIdsSnapshot.serialize(dataOutputStream);
+ byte[] bytes = docIds.toBytes();
Review Comment:
Yeah I haven't seen any longer pause from the small benchmark I have taken . Older code was completely cloning the bitmap. Atleast with that we reduce the number of lock contentions as well as bytes which are eligible for gc immediately. With off-heap memory:
It's might not be worth it here:
Direct buffers are slower to allocate. ByteBuffer.allocateDirect() requires a system call (mmap) and has higher per-allocation overhead. For a ~40KB buffer created per segment, this would add overhead dominate any benefit. For maybe larger bitmaps it might make sense but it doesn't explain the purpose. We might just use it to save the heap memory and bring down heap overhead or long gc pauses. I don't think in our scenario we had a long gc pause. @KKcorps correct me if I am wrong.
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3010866941
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java:
##########
@@ -46,7 +46,6 @@ public LazyRow() {
}
public void init(IndexSegment segment, int docId) {
- clear();
Review Comment:
init is called whenever we call the previous row, and then merge with that row. Since we are anyway clearing this map manually after the merge, thought this might be duplicate and unnecessary to clear() again.
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3011530558
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -962,24 +966,18 @@ protected void doTakeSnapshot() {
continue;
}
ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
- immutableSegment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getValidDocIds();
- immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- validDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap = immutableSegment.getValidDocIds();
+ immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME, validDocIdsBitMap);
if (_deleteRecordColumn != null) {
- queryableDocIdsBitMap =
- immutableSegment.getQueryableDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getQueryableDocIds();
+ queryableDocIdsBitMap = immutableSegment.getQueryableDocIds();
immutableSegment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
queryableDocIdsBitMap);
}
_updatedSegmentsSinceLastSnapshot.remove(segment);
numImmutableSegments++;
- numPrimaryKeysInSnapshot += validDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
+ numPrimaryKeysInSnapshot += validDocIdsBitMap.getCardinality();
Review Comment:
We are reading bytes and cardinality in multiple places, and updates is actually possible between these reads (e.g. one segment removed by retention manager). To ensure consistency, we might want to use a snapshot clone
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -962,24 +966,18 @@ protected void doTakeSnapshot() {
continue;
}
ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
- immutableSegment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getValidDocIds();
- immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- validDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap = immutableSegment.getValidDocIds();
Review Comment:
I feel we might still want to have the null check. In case segment contains an invalid snapshot, it will have `null` bitmap but still has snapshot file
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -160,13 +159,14 @@ public void persistDocIdsSnapshot(String fileName, ThreadSafeMutableRoaringBitma
LOGGER.warn("Previous snapshot was not taken cleanly. Remove tmp file: {}", tmpFile);
FileUtils.deleteQuietly(tmpFile);
}
- MutableRoaringBitmap docIdsSnapshot = docIds.getMutableRoaringBitmap();
- try (DataOutputStream dataOutputStream = new DataOutputStream(new FileOutputStream(tmpFile))) {
- docIdsSnapshot.serialize(dataOutputStream);
+ byte[] bytes = docIds.toBytes();
Review Comment:
Can you benchmark the performance of serialization vs clone? Will this cause longer pause?
Also, will this save heap memory? Should we consider serializing using off-heap memory?
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3013582689
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -564,6 +564,7 @@ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
_reusePreviousRow.init(currentSegment, currentDocId);
_partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
_reuseMergeResultHolder.clear();
+ _reusePreviousRow.clear();
Review Comment:
Same as in the non-consistent-deletes manager: `_reusePreviousRow.clear()` (and `_reuseMergeResultHolder.clear()`) should be in a finally block so the reusable `LazyRow` doesn't retain the segment reference/cached values if `_partialUpsertHandler.merge(...)` throws.
```suggestion
try {
_partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
} finally {
_reuseMergeResultHolder.clear();
_reusePreviousRow.clear();
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -431,6 +431,7 @@ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
_reusePreviousRow.init(currentSegment, currentDocId);
_partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
_reuseMergeResultHolder.clear();
+ _reusePreviousRow.clear();
Review Comment:
`_reusePreviousRow.clear()` (and `_reuseMergeResultHolder.clear()`) are only executed when `_partialUpsertHandler.merge(...)` returns normally. If merge throws, the reusable `LazyRow` can keep holding the previous segment reference and cached values, reintroducing the memory retention issue. Wrap the merge/clear sequence in a try/finally to guarantee cleanup.
```suggestion
try {
_partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
} finally {
_reuseMergeResultHolder.clear();
_reusePreviousRow.clear();
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -962,24 +966,18 @@ protected void doTakeSnapshot() {
continue;
}
ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
- immutableSegment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getValidDocIds();
- immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- validDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap = immutableSegment.getValidDocIds();
+ immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME, validDocIdsBitMap);
if (_deleteRecordColumn != null) {
- queryableDocIdsBitMap =
- immutableSegment.getQueryableDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getQueryableDocIds();
+ queryableDocIdsBitMap = immutableSegment.getQueryableDocIds();
immutableSegment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
queryableDocIdsBitMap);
}
Review Comment:
`ImmutableSegmentImpl.getValidDocIds()` / `getQueryableDocIds()` are annotated @Nullable, but this snapshot path now persists them unconditionally. If either bitmap is null (e.g., segment hasn't had upsert enabled or TTL handling left it unset), this will NPE inside `persistDocIdsSnapshot()` and/or when calling `getCardinality()`. Consider adding explicit null handling here (skip snapshot for that bitmap, or fail fast with a clear message) consistent with the null checks added in the `segmentsWithoutSnapshot` loop.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java:
##########
@@ -46,7 +46,6 @@ public LazyRow() {
}
public void init(IndexSegment segment, int docId) {
Review Comment:
`init()` no longer clears the cached field/null-value state. Since `getValue()` memoizes per-column results, reusing a single `LazyRow` instance across different docIds/segments without calling `clear()` first can return stale values from the previous row. This also contradicts the class-level Javadoc that says reusing is done by re-initializing via `init(...)`. Consider restoring `clear()` at the start of `init(...)` (while still keeping the new post-merge `clear()` calls for eager segment reference cleanup), or otherwise updating callers + Javadoc so `init(...)` cannot be misused.
```suggestion
public void init(IndexSegment segment, int docId) {
clear();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1004,21 +1002,23 @@ protected void doTakeSnapshot() {
}
try {
ThreadSafeMutableRoaringBitmap segmentQueryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap segmentValidDocIdsBitMap =
- segment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap() : segment.getValidDocIds();
- segment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME, segmentValidDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap segmentValidDocIdsBitMap = segment.getValidDocIds();
+ // segment that is out of TTL with no valid docId snapshot can be skipped while taking snapshot
+ // i.e such segments will have null bitmap
+ if (segmentValidDocIdsBitMap != null) {
+ segment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME, segmentValidDocIdsBitMap);
+ numPrimaryKeysInSnapshot += segmentValidDocIdsBitMap.getCardinality();
+ }
if (_deleteRecordColumn != null) {
- segmentQueryableDocIdsBitMap = segment.getQueryableDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : segment.getQueryableDocIds();
- segment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
- segmentQueryableDocIdsBitMap);
+ segmentQueryableDocIdsBitMap = segment.getQueryableDocIds();
+ if (segmentQueryableDocIdsBitMap != null) {
+ segment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
+ segmentQueryableDocIdsBitMap);
+ numQueryableDocIdsInSnapshot += segmentQueryableDocIdsBitMap.getCardinality();
+ }
}
_updatedSegmentsSinceLastSnapshot.remove(segment);
numImmutableSegments++;
- numPrimaryKeysInSnapshot += segmentValidDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
- if (_deleteRecordColumn != null) {
- numQueryableDocIdsInSnapshot += segmentQueryableDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
- }
} catch (Exception e) {
Review Comment:
In the `segmentsWithoutSnapshot` loop, `numImmutableSegments++` and `_updatedSegmentsSinceLastSnapshot.remove(segment)` happen even when `segmentValidDocIdsBitMap` / `segmentQueryableDocIdsBitMap` are null and no snapshot files are written. This can skew snapshot metrics/logging and also drops the segment from the "needs snapshot" tracking, preventing a retry later. Consider only counting/removing the segment from `_updatedSegmentsSinceLastSnapshot` when at least one snapshot file is actually persisted (or explicitly persisting an empty snapshot when that’s the desired behavior).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -160,13 +159,14 @@ public void persistDocIdsSnapshot(String fileName, ThreadSafeMutableRoaringBitma
LOGGER.warn("Previous snapshot was not taken cleanly. Remove tmp file: {}", tmpFile);
FileUtils.deleteQuietly(tmpFile);
}
- MutableRoaringBitmap docIdsSnapshot = docIds.getMutableRoaringBitmap();
- try (DataOutputStream dataOutputStream = new DataOutputStream(new FileOutputStream(tmpFile))) {
- docIdsSnapshot.serialize(dataOutputStream);
+ byte[] bytes = docIds.toBytes();
+ int cardinality = docIds.getCardinality();
Review Comment:
`bytes` and `cardinality` are fetched via two separate synchronized calls on `docIds`, so the logged cardinality can drift from the serialized snapshot if the bitmap is mutated between calls. Consider capturing both under a single lock (e.g., `synchronized (docIds) { ... }`) or adding a single method that returns both snapshot bytes and cardinality atomically.
```suggestion
byte[] bytes;
int cardinality;
synchronized (docIds) {
// Capture snapshot bytes and cardinality from the same bitmap state
bytes = docIds.toBytes();
cardinality = docIds.getCardinality();
}
```
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3009701273
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -962,24 +966,18 @@ protected void doTakeSnapshot() {
continue;
}
ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
- ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
- immutableSegment.getValidDocIds() == null ? new ThreadSafeMutableRoaringBitmap()
- : immutableSegment.getValidDocIds();
- immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- validDocIdsBitMap);
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap = immutableSegment.getValidDocIds();
Review Comment:
Why is null check removed here?
--
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
Re: [PR] Upserts Bitmap Optimizations while taking Snapshot - Segment Commit [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #18020:
URL: https://github.com/apache/pinot/pull/18020#discussion_r3009696665
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java:
##########
@@ -46,7 +46,6 @@ public LazyRow() {
}
public void init(IndexSegment segment, int docId) {
- clear();
Review Comment:
Why have we removed this? are we going to rely on manually clearing this map ourselves ?
--
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