You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "priyen-stripe (via GitHub)" <gi...@apache.org> on 2026/05/05 19:04:10 UTC
[PR] Add per-table insert/update counters to upsert metadata managers [pinot]
priyen-stripe opened a new pull request, #18427:
URL: https://github.com/apache/pinot/pull/18427
## Summary
Adds two new `ServerMeter` entries and instruments `doAddRecord()` in both
`ConcurrentMapPartitionUpsertMetadataManager` and
`ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes` to emit
per-table counters that distinguish append (new PK) from update (existing PK)
traffic in upsert tables. Out-of-order records are counted by neither metric,
consistent with the existing `UPSERT_OUT_OF_ORDER` meter.
New metric names:
- UPSERT_NEW_KEYS_INSERTED and UPSERT_EXISTING_KEYS_UPDATED
## Testing
Tested internally at Stripe on test tables, worked:
<img width="1459" height="191" alt="image" src="https://github.com/user-attachments/assets/895fa63e-0aed-4874-a089-c62b2afa1a58" />
## Label
`observability`
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #18427:
URL: https://github.com/apache/pinot/pull/18427#issuecomment-4836976750
cc @xiangfu0
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #18427:
URL: https://github.com/apache/pinot/pull/18427#issuecomment-4390329171
@xiangfu0 thanks for the review, address your feedback
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #18427:
URL: https://github.com/apache/pinot/pull/18427#issuecomment-4382187468
cc @Jackie-Jiang @suvodeep-pyne
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18427:
URL: https://github.com/apache/pinot/pull/18427#discussion_r3717058511
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1229,6 +1229,16 @@ protected void removeDocId(IndexSegment segment, int docId) {
trackUpdatedSegmentsSinceLastSnapshot(segment);
}
+ protected void emitUpsertMetrics(RecordInfo recordInfo, boolean isNewKey) {
+ if (recordInfo.isDeleteRecord()) {
Review Comment:
Do we want to track the case where deletion is trying to delete a non-existing key?
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #18427:
URL: https://github.com/apache/pinot/pull/18427#issuecomment-4721854311
cc @xiangfu0
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #18427:
URL: https://github.com/apache/pinot/pull/18427#discussion_r3196524353
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -404,11 +405,17 @@ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
}
} else {
// New primary key
+ isNewKey.set(true);
addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
return new RecordLocation(segment, newDocId, newComparisonValue);
}
});
+ if (isNewKey.get()) {
Review Comment:
good catch, pushed commit to fix this
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -537,11 +538,17 @@ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
}
} else {
// New primary key
+ isNewKey.set(true);
addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
return new RecordLocation(segment, newDocId, newComparisonValue, 1);
}
});
+ if (isNewKey.get()) {
Review Comment:
good catch, pushed commit to fix 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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #18427:
URL: https://github.com/apache/pinot/pull/18427#issuecomment-4422455036
cc @xiangfu0
--
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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18427:
URL: https://github.com/apache/pinot/pull/18427#issuecomment-4382662901
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18427?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 `14.28571%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.64%. Comparing base ([`b870804`](https://app.codecov.io/gh/apache/pinot/commit/b870804e2028d140e8fed8213ca6f14ecc708322?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a88498d`](https://app.codecov.io/gh/apache/pinot/commit/a88498df3d495aaec935cf6f6ecee9050b413a3e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18427?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/18427?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) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tionUpsertMetadataManagerForConsistentDeletes.java](https://app.codecov.io/gh/apache/pinot/pull/18427?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2FConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29uY3VycmVudE1hcFBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlckZvckNvbnNpc3RlbnREZWxldGVzLmphdmE=) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18427?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 (b870804) and HEAD (a88498d). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (b870804) | HEAD (a88498d) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|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 #18427 +/- ##
============================================
- Coverage 63.61% 55.64% -7.98%
+ Complexity 1717 860 -857
============================================
Files 3252 2541 -711
Lines 199051 147621 -51430
Branches 30838 23797 -7041
============================================
- Hits 126618 82137 -44481
+ Misses 62352 58466 -3886
+ Partials 10081 7018 -3063
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18427/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/18427/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/18427/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/18427/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/18427/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/18427/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.64% <14.28%> (-7.98%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18427/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.64% <14.28%> (-7.98%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18427/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.64% <14.28%> (-7.97%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18427/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.64% <14.28%> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18427/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/18427?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] Add per-table insert/update counters to upsert metadata managers [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18427:
URL: https://github.com/apache/pinot/pull/18427#discussion_r3195307998
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -537,11 +538,17 @@ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
}
} else {
// New primary key
+ isNewKey.set(true);
addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
return new RecordLocation(segment, newDocId, newComparisonValue, 1);
}
});
+ if (isNewKey.get()) {
Review Comment:
Same issue here: delete records are counted under the new insert/update meters. In delete-enabled tables this misclassifies tombstone traffic as inserts/updates, so the exposed per-table counters become incorrect even though the underlying upsert state is handled correctly. This needs the same `!recordInfo.isDeleteRecord()` guard or a dedicated delete counter.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -404,11 +405,17 @@ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
}
} else {
// New primary key
+ isNewKey.set(true);
addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
return new RecordLocation(segment, newDocId, newComparisonValue);
}
});
+ if (isNewKey.get()) {
Review Comment:
This block increments the new meters for every non-out-of-order record, including tombstones. On delete-enabled upsert tables, a first-seen delete will hit `UPSERT_NEW_KEYS_INSERTED` and a delete for an existing PK will hit `UPSERT_EXISTING_KEYS_UPDATED`, even though neither is an insert/update row. Because these are new exported Prometheus counters, that makes the public metric contract wrong for delete-heavy workloads. Please guard these counters with `!recordInfo.isDeleteRecord()` or add a separate delete meter.
--
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