deepthi912 commented on PR #19832: URL: https://github.com/apache/pinot/pull/19832#issuecomment-6200990044
Could we make `_persistedWatermark` `protected`, or expose a `protected double getPersistedWatermark()`? A getter would be better than the field — it keeps the computation in `getWatermarkToPersist()` as the single source. The reason: StarTree's RocksDB upsert manager caps its TTL cleanup watermark the same way, currently `min(serverWatermark, minionSnapshotWatermark)`. That covers preload/restore, but not the server-local validDocIds snapshot, which is what `UpsertCompactionTask` and SegmentRefreshTask actually read — `TablesResource.getValidDocIds` resolves `SNAPSHOT` via `loadDocIdsFromSnapshot(...)`, not the live bitmap. `_persistedWatermark` is exactly the missing third term. Related, and possibly worth folding in here: `doRemoveExpiredPrimaryKeys` still expires against `_largestSeenComparisonValue`. For `metadataTTL` the new `canSkipSegmentOutOfTTL` covers it on the add side, but `deletedKeysTTL` is not covered — a key expired there sits in a segment typically still *within* `metadataTTL`, so the segment is added normally and the retained row revives the key. It can then outrank a newer record that arrived with a lower comparison value. -- 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]
