AkshayUpadhyay2411 opened a new pull request, #19832:
URL: https://github.com/apache/pinot/pull/19832
Fixes #19831
## Problem
On a full upsert table with `metadataTTL` and snapshots enabled, a restarted
server can return duplicate rows for a
primary key, on that replica only, until the segments are reloaded.
The snapshot pass takes each segment's lock with `tryLock()` and skips the
segment when the lock is held, e.g. by a
segment reload or replace. The segment keeps a stale snapshot, or none at
all, but the TTL watermark is still
persisted. After a restart, the segment is out of TTL and is added with the
stale snapshot, or with all docs valid
when there is no snapshot. Its superseded rows show up again.
## Change
`BasePartitionUpsertMetadataManager`:
1. **Cap the persisted watermark.** After a snapshot pass, every segment
still tracked as updated has a stale or
missing snapshot. The persisted watermark is capped to keep those
segments within TTL. A just committed segment
that misses its snapshot (expected while its lock is still held by the
replacing thread) is within TTL anyway, so
it does not hold the watermark back. The watermark in memory, used to
expire primary keys, is unchanged.
2. **Only trust a snapshot of a segment out of TTL against the persisted
watermark.** Adding or preloading a segment
as out of TTL (taking its snapshot as is) now also requires it to be out
of TTL against the last persisted
watermark. Otherwise it goes through the regular flow, which resolves the
primary keys by comparison value. This
is needed because the watermark in memory is bumped by every segment
added during a restart, and makes the result
independent of the order the segments are loaded in.
`stop()` is unchanged, and neither part depends on a graceful shutdown.
## Compatibility and cost
- Server only, no config, wire or on-disk format change; rolling upgrade
safe.
- No change for tables without `metadataTTL`.
- The watermark file persisted by an older version is trusted as is, so the
restart that upgrades a server can still
hit the issue once if the last snapshot pass before it missed a segment. A
reload fixes it; from then on the
persisted watermark is capped.
- When segments keep missing their snapshot, the persisted watermark stays
behind them, so after a restart more
segments go through the regular flow, with their primary keys loaded until
the next expiry pass. A warning is
logged: `Capped TTL watermark to persist from: ... to: ...`.
## Testing
- `UpsertSnapshotRestartTest` (new): restarts through add and preload in
both load orders, with no contention, a
missing snapshot and a stale snapshot; the watermark advancing once the
missed snapshot is taken; a missed snapshot
within TTL not capping the watermark; and `stop()` not snapshotting
uncommitted updates. The restart cases fail
without the fix, and some still fail with either part of the change alone.
- `UpsertSnapshotRestartIntegrationTest` (new): holds a segment's lock while
the next segments commit so the snapshot
passes skip it, restarts the server, and checks one row per primary key,
also after a reload. Fails without the fix.
- Existing upsert, dedup, snapshot and TTL tests in `pinot-segment-local`
and `pinot-core`, and the upsert and dedup
integration tests.
## Related
#9062, #10928, #12232 (validDocIds snapshots), #18860 (reload from snapshot
for TTL tables), #19512 (watermark bump
after segment add)
--
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]
