Jackie-Jiang commented on code in PR #19028:
URL: https://github.com/apache/pinot/pull/19028#discussion_r3633225153
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -577,15 +592,22 @@ public void replaceSegment(ImmutableSegment segment,
IndexSegment oldSegment) {
}
try {
doReplaceSegment(segment, oldSegment);
- if (!(segment instanceof EmptyIndexSegment)) {
+ if (segment instanceof ImmutableSegmentImpl) {
+ ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
_trackedSegments.add(segment);
if (_enableSnapshot) {
- _updatedSegmentsSinceLastSnapshot.add(segment);
+ _updatedSegmentsSinceLastSnapshot.add(immutableSegment);
Review Comment:
Good catch. Fixed by swapping the publication order in
`trackSegmentForSnapshot()`: the segment is added to `_segmentsWithSnapshot`
before `_updatedSegmentsSinceLastSnapshot`. The snapshot flow checks the
updated set before the with-snapshot set, and a successful read of a
`ConcurrentHashMap` entry happens-after the put that created it, so observing
the updated marker guarantees observing the with-snapshot membership as well —
the misclassification window is closed, not just narrowed. The same ordering
applies to `addSegment`/`preloadSegment`, which share the helper.
Two notes:
- The window was narrower than described: the old segment object stays in
`_trackedSegments`, `_updatedSegmentsSinceLastSnapshot` and
`_segmentsWithSnapshot` until `untrackSegment(oldSegment)` runs (after the new
object is fully published), so in most interleavings it is classified into the
first loop and gates the second loop via the failed `tryLock()` or the tracked
re-check. The harmful case additionally requires the weakly consistent iterator
to miss the old object.
- On the deterministic interleaving test: with the ordering fix, the
misclassified state can no longer be observed, so a test would either need to
force the sets into a state the fix makes impossible, or add injection seams
(latches) into the production helper to freeze it between the two publications.
Neither seems worth it for a pure publication-order fix, but happy to add a
seam if you feel strongly.
The sibling window where the new object is observed before the updated
marker is published (classified as unchanged, no gate) exists on master with
the same publication order, and is tracked separately with the other
pre-existing classification races.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -577,15 +592,22 @@ public void replaceSegment(ImmutableSegment segment,
IndexSegment oldSegment) {
}
try {
doReplaceSegment(segment, oldSegment);
- if (!(segment instanceof EmptyIndexSegment)) {
+ if (segment instanceof ImmutableSegmentImpl) {
+ ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
_trackedSegments.add(segment);
if (_enableSnapshot) {
- _updatedSegmentsSinceLastSnapshot.add(segment);
+ _updatedSegmentsSinceLastSnapshot.add(immutableSegment);
Review Comment:
Done, extracted `trackSegmentForSnapshot()` and reused it in
`addSegment`/`preloadSegment`/`replaceSegment`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -577,15 +592,22 @@ public void replaceSegment(ImmutableSegment segment,
IndexSegment oldSegment) {
}
try {
doReplaceSegment(segment, oldSegment);
- if (!(segment instanceof EmptyIndexSegment)) {
+ if (segment instanceof ImmutableSegmentImpl) {
+ ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
_trackedSegments.add(segment);
if (_enableSnapshot) {
- _updatedSegmentsSinceLastSnapshot.add(segment);
+ _updatedSegmentsSinceLastSnapshot.add(immutableSegment);
+ // Evaluate the snapshot file on the new segment object because the
replacement might have kept it
+ // (e.g. segment reload) or started from a clean segment directory
(e.g. segment re-download).
+ if (hasValidDocIdsSnapshotFile(immutableSegment)) {
+ _segmentsWithSnapshot.add(immutableSegment);
+ }
}
}
_trackedSegments.remove(oldSegment);
Review Comment:
Done, extracted `untrackSegment()` and reused it in
`replaceSegment`/`removeSegment`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1006,26 +1046,38 @@ protected void doTakeSnapshot() {
}
}
// If we have skipped any segments in the previous for-loop, we should
skip the next for-loop, basically to not
- // add new snapshot files on disk. This ensures all the validDocIds &
queryable docIds snapshots kept on disk are
- // still disjoint with each other, although some of them may have become
stale, i.e. tracking more valid docs than
- // expected.
+ // add new snapshot files on disk. This ensures all the validDocIds
snapshots kept on disk are still disjoint
+ // with each other, although some of them may have become stale, i.e.
tracking more valid docs than expected.
if (!isSegmentSkipped) {
Review Comment:
Sounds good. Note that persistent lock contention on a segment only defers
that one segment (it stays in `_updatedSegmentsSinceLastSnapshot` and is
retried every snapshot round), and the skips are observable via
`ServerMeter.UPSERT_MISSED_VALID_DOC_ID_SNAPSHOT_COUNT`, so we can watch
exactly this signal when testing in the clusters.
--
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]
