You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tarun11Mavani (via GitHub)" <gi...@apache.org> on 2026/06/11 08:18:07 UTC
[PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
tarun11Mavani opened a new pull request, #18733:
URL: https://github.com/apache/pinot/pull/18733
### Problem
When `upsertMetadataTTL` is configured, expired metadata entries are only cleaned up at segment commit time (which can be hours apart for low-throughput tables). Records arriving after TTL expiry are still compared against stale metadata — a record with a lower comparison value gets rejected as out-of-order even though the existing entry is expired and should be irrelevant.
### Fix
Add an inline TTL expiry check in `doAddRecord()`. When the existing record's comparison value is below the TTL threshold (`largestSeenComparisonValue - metadataTTL`), skip the comparison entirely:
- The new record always wins regardless of its comparison value
- The old doc is invalidated via `replaceDocId` (no duplicate queryable rows)
- The map entry is replaced with a fresh `RecordLocation`
Also guard the partial upsert merge in `doUpdateRecord()` — skip reading the previous row when it's expired, preventing stale data from being merged into the new record.
### Changes
- `ConcurrentMapPartitionUpsertMetadataManager.doAddRecord()` — TTL check before comparison
- `ConcurrentMapPartitionUpsertMetadataManager.doUpdateRecord()` — TTL guard on partial upsert merge
- Test: expired metadata is skipped, lower comparison value accepted, old doc invalidated
### Not changed
- `ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes` — `metadataTTL` and `enableDeletedKeysCompactionConsistency` are mutually exclusive, so the check can never fire there
- `removeExpiredPrimaryKeys()` — lazy cleanup at segment commit continues as-is
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-4750675085
@xiangfu0 @Jackie-Jiang gentle bump on this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-5114524814
> I don't follow the scenario. When you encounter this issue, it means the existing record is out of TTL, and the new record is also out of TTL (it is even older than the existing record)?
Not quite — TTL is only evaluated against the *existing stored* metadata, not the new record. There's no TTL check on the incoming record itself.
Concretely (this is the scenario the new test covers):
- PK=0 is stored with comparisonValue=1000.
- PK=1 arrives with comparisonValue=1200, which becomes the new watermark (`_largestSeenComparisonValue`).
- With `metadataTTL=100`, the threshold is `1200-100=1100`. PK=0's stored value (1000) is now below that threshold, so its metadata is expired — even though 1000 isn't "old" in any absolute sense, just relative to the current watermark.
- Now a new record for PK=0 arrives with comparisonValue=900 — lower than the stored 1000. Today this gets rejected as out-of-order (900 < 1000). With the fix, since the *existing* entry is already expired, we skip the comparison entirely and accept the new record unconditionally, treating it as if PK=0 had never been seen.
So the new record doesn't need to be "out of TTL" — it just needs to arrive after the existing entry has fallen behind the watermark by more than `metadataTTL`. For partial-upsert tables this also matters for merging: if the previous entry is expired, we skip merging the new record's unset fields against that stale previous row, since that data is considered gone.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-4679128729
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18733?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `88.88889%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 37.03%. Comparing base ([`3693493`](https://app.codecov.io/gh/apache/pinot/commit/36934936ba425b7fe8b83893aedf065fe29dc848?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5085afb`](https://app.codecov.io/gh/apache/pinot/commit/5085afb0e358ca0a8f783eff120124ae54a83847?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 70 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18733?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...t/ConcurrentMapPartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/18733?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2FConcurrentMapPartitionUpsertMetadataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29uY3VycmVudE1hcFBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 88.88% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18733?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (3693493) and HEAD (5085afb). Click for more details.
>
> <details><summary>HEAD has 13 uploads less than BASE</summary>
>
>| Flag | BASE (3693493) | HEAD (5085afb) |
>|------|------|------|
>|java-21|4|1|
>|unittests1|1|0|
>|temurin|4|1|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18733 +/- ##
=============================================
- Coverage 56.80% 37.03% -19.78%
- Complexity 7 1298 +1291
=============================================
Files 2580 3380 +800
Lines 149652 209651 +59999
Branches 24180 32779 +8599
=============================================
- Hits 85010 77641 -7369
- Misses 57444 124956 +67512
+ Partials 7198 7054 -144
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.03% <88.88%> (-19.78%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.03% <88.88%> (-19.78%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.03% <88.88%> (-19.77%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.03% <88.88%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18733?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-5145884057
> So the new record doesn't need to be "out of TTL" — it just needs to arrive after the existing entry has fallen behind the watermark by more than `metadataTTL`. For partial-upsert tables this also matters for merging: if the previous entry is expired, we skip merging the new record's unset fields against that stale previous row, since that data is considered gone.
The definition of `out of TTL` is it is older than watermark - `metadataTTL`, so this is out of TTL. The scenario you described should be forbidden. It is not only an out-of-order event, but so stale that the lag is more than the TTL.
When `metadataTTL` is used, a record shouldn't be updated after this time (e.g. 2 days), or the behavior is undefined.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-6336601328
This comes from a model of upsert thread orders that StarTree runs on upsert PRs.
**This clears none of the model's known bad orders and adds one: the row with a lower comparison value that `doAddRecord` now accepts for an expired key is undone when its segment is committed or loaded again.**
I replayed the model's 45 known bad orders with and without the change, and none changed. A 62 minute search, 402 runs over 10 metadataTTL table shapes, then found 44 runs that break only with the change. All but one are this order, and the last is a smaller merge edge after a consuming segment is reset.
**The order it adds.** Only the consuming path has the new branch. `doAddOrReplaceSegment` still compares, so at commit the expired row takes the key back. It needs no race, only a late row for a key that is already past metadataTTL.
```mermaid
sequenceDiagram
participant S as Stream
participant C as Consuming replica
participant L as Server loading N1
S->>C: A at 4, watermark is now 4
S->>C: B at 1, doc 0 of segment N1
S->>C: B at 0, doc 1 of N1
C->>C: B at 1 is below 4 minus metadataTTL 2, so doc 1 wins with no compare
Note over C: serves B at 0
C->>L: N1 commits, then replaceSegment or addSegment walks its docs
L->>L: doc 0, B at 1, takes the key
L->>L: doc 1, B at 0, loses the compare to doc 0
Note over L: serves B at 1 again
```
In the new test the late row lands in a later segment, so the commit keeps it. A new replica, or a restart that does not preload from an up to date snapshot, still serves the expired row, and replicas disagree until the key gets a newer row.
<details><summary>Model history</summary>
```
('append', 'A', (4,), 0, {'v': 0}, False)
('commit_remote', 0, 0)
('append', 'B', (1,), 1, {'v': 0}, False)
('append', 'B', (0,), 2, {'v': 0}, False)
('commit_remote', 1, 2)
('add_immutable', 'N0')
('step', 'add_immutable', 'N0', 'start_add')
('step', 'add_immutable', 'N0', 'add_row:A@0')
('add_immutable', 'N1')
('append', 'A', (0,), 3, {'v': 0}, False)
('step', 'add_immutable', 'N0', 'register')
('step', 'add_immutable', 'N1', 'start_add')
('step', 'add_immutable', 'N1', 'add_row:B@1')
('step', 'add_immutable', 'N1', 'add_row:B@2')
('step', 'add_immutable', 'N1', 'register')
```
</details>
The model covers one replica and the handoff slice of one partition, and it searched only metadataTTL tables.
<!-- upsert-model-check head=5085afb0e358ca0a8f783eff120124ae54a83847 -->
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-5145897232
I'm okay dropping the record when its comparison value is older than the threshold, but we definitely shouldn't treat it as valid
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18733:
URL: https://github.com/apache/pinot/pull/18733#discussion_r3538972656
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -423,7 +436,9 @@ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
// - New record is not out-of-order
// - Previous record is not deleted
if (!recordInfo.isDeleteRecord()
- && recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue()) >= 0) {
+ && recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue()) >= 0
+ && !(_metadataTTL > 0 && isOutOfMetadataTTL(
+ ((Number) recordLocation.getComparisonValue()).doubleValue()))) {
Review Comment:
The TTL guard in doUpdateRecord() checks expiration via isOutOfMetadataTTL(recordLocationValue), which uses _largestSeenComparisonValue. But updateRecord() is invoked before addRecord() (see MutableSegmentImpl#index() flow), so _largestSeenComparisonValue may not yet include the current record’s comparison value. This can still merge a stale previous row when the current record advances the watermark enough to expire it (exact scenario the PR is trying to prevent).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -378,6 +378,19 @@ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
if (currentRecordLocation != null) {
// Existing primary key
IndexSegment currentSegment = currentRecordLocation.getSegment();
+
+ // If existing metadata is expired per TTL, treat as new key
+ if (_metadataTTL > 0 && isOutOfMetadataTTL(
+ ((Number) currentRecordLocation.getComparisonValue()).doubleValue())) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ }
Review Comment:
In the new "expired metadata" branch, when replacing a location from a different (often immutable) segment, the code doesn’t preserve the previous RecordLocation in _previousKeyToRecordLocationMap. The normal replacement path does this when isTableTypeInconsistentDuringConsumption() is enabled so revertAndRemoveSegment() can restore keys if the consuming segment is rolled back/removed. The TTL replacement should mirror that bookkeeping for the same inconsistency mode.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(upsert): skip expired metadata during upsert instead of comparing against it [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18733:
URL: https://github.com/apache/pinot/pull/18733#issuecomment-4912316800
Traced both branches. The out-of-order fix in doAddRecord is real and well-scoped in intent: versus today it only rescues a record whose value is < the stored one when that stored entry is out-of-TTL (the exact rejection this fixes).
One correction though - it is not a clean no-op on the >= path. The expired branch runs whenever the entry is out-of-TTL, regardless of comparison order, so when new >= current AND expired it now handles the record instead of the normal >= branch. In the cross-segment isTableTypeInconsistentDuringConsumption() case that means it skips the _previousKeyToRecordLocationMap.put(primaryKey, currentRecordLocation) bookkeeping the >= branch does (which revertAndRemoveSegment relies on). That is exactly Copilot's L390 point and worth addressing rather than dismissing, since the expired branch subsumes the happy path too, not just the buggy one.
Parts that do check out: the leading `_metadataTTL > 0 &&` is load-bearing, not redundant with isOutOfMetadataTTL's own guard - it short-circuits the `(Number) getComparisonValue()` cast in the argument, so non-TTL tables with non-numeric comparison columns cannot ClassCastException. And skipping the ForConsistentDeletes subclass is fine: TableConfigUtils forbids metadataTTL alongside enableDeletedKeysCompactionConsistency, so the branch is unreachable there.
Coverage gap: the new test only drives doAddRecord. The doUpdateRecord guard has no test and is the riskier half - updateRecord() runs before addRecord() bumps _largestSeenComparisonValue (doAddRecord updates the watermark at the top; doUpdateRecord does not), so its expiry check can read a watermark one record behind doAddRecord's and still merge a stale row. A partial-upsert record against an expired previous row would close the gap and settle whether that branch actually skips the merge. Also worth a rebase to re-trigger CI - the green run is fairly far behind master.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org