xiangfu0 commented on code in PR #20204:
URL: https://github.com/apache/pinot/pull/20204#discussion_r4523595980
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -214,7 +214,19 @@ protected BasePartitionUpsertMetadataManager(String
tableNameWithType, int parti
_largestSeenComparisonValue = new AtomicDouble(TTL_WATERMARK_NOT_SET);
WatermarkUtils.deleteWatermark(getWatermarkFile());
}
- _persistedWatermark = _largestSeenComparisonValue.get();
+ _persistedWatermark = loadPersistedWatermark();
+ }
+
+ /// A watermark file without the capped version was written before the
watermark got capped, so it can be ahead of
+ /// segments with stale snapshots. Don't skip any segment against it until
the next snapshot persists a capped one.
+ private double loadPersistedWatermark() {
+ double watermark = _largestSeenComparisonValue.get();
+ if (_metadataTTL > 0 && watermark != TTL_WATERMARK_NOT_SET
+ && WatermarkUtils.loadWatermarkVersion(getWatermarkFile()) !=
WatermarkUtils.CAPPED_WATERMARK_VERSION) {
Review Comment:
#19832 is not in any release; `release-1.6.0` has no
`getWatermarkToPersist`. So every table with metadataTTL hits this path once
when it upgrades from a release.
On that restart, every out-of-TTL segment is added in full.
`removeExpiredPrimaryKeys()` only runs when a consuming segment commits
(`RealtimeSegmentDataManager:918`), so the primary-key map holds every
historical key until then. That could be a large heap and restart-time spike on
servers sized for the TTL window. Consider an upgrade note, or expiring keys
once after the partition finishes loading. Not blocking.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java:
##########
@@ -331,6 +332,34 @@ public void testManageWatermark()
upsertMetadataManager.close();
}
+ @Test
+ public void testUncappedWatermarkIsNotTrustedToSkipSegments()
+ throws IOException {
+ _contextBuilder.setEnableSnapshot(true).setMetadataTTL(30);
+ File watermarkFile = new File(INDEX_DIR,
V1Constants.TTL_WATERMARK_TABLE_PARTITION + 0);
+
+ // A watermark persisted before capping keeps expiring keys, but no
segment is skipped against it on load
+ WatermarkUtils.persistWatermark(290, watermarkFile,
WatermarkUtils.NO_WATERMARK_VERSION);
Review Comment:
This test only checks `canSkipSegmentOutOfTTL`. It would be good to also add
old-writer cases to `UpsertSnapshotRestartTest`: write an unmarked 290 after a
stale or missing segA snapshot, then call `restart(...)`. I tried exactly that
locally. It fails on the base commit and passes here, so it guards the
end-to-end fix in OSS. You'd need to relax the `persistedWatermark(manager)`
assertion in `restart()` for unmarked files.
--
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]
