klsince commented on code in PR #13285:
URL: https://github.com/apache/pinot/pull/13285#discussion_r1630013857
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -874,19 +875,32 @@ protected void doTakeSnapshot() {
numConsumingSegments++;
continue;
}
- ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
- if (!immutableSegment.hasValidDocIdsSnapshotFile()) {
- segmentsWithoutSnapshot.add(immutableSegment);
+ if (!_updatedSegmentsSinceLastSnapshot.contains(segment)) {
Review Comment:
If we enableSnapshot on an existing upsert table and the set is empty, I
think we would skip those existing segments until they get updated or possibly
some of them never get updated in long future and they won't have snapshot on
disk. (This was a bug I hit on while testing the
`_updatedSegmentsSinceLastRefresh`)
We would need to take snapshot for all tracked segments for the first time
when to take snapshot for the partition, and then start to skip segments not
having updates with this Set.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1111,6 +1132,9 @@ protected void addDocId(IndexSegment segment,
ThreadSafeMutableRoaringBitmap val
try {
doAddDocId(validDocIds, queryableDocIds, docId, recordInfo);
} finally {
+ if (_enableSnapshot) {
Review Comment:
I'd suggest to move this if-block outside the critical section of
upsertViewLock
btw, it may help to comment that we don't need to take snasphotLock when
updating this Set (IIUC)
--
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]
