feat(upsert): add primary-key map size-in-bytes gauge#18980
feat(upsert): add primary-key map size-in-bytes gauge#18980tarun11Mavani wants to merge 2 commits into
Conversation
Add UPSERT_PRIMARY_KEY_MAP_SIZE_IN_BYTES ServerGauge to complement the existing key-count gauge. Tables with few large-string PKs and tables with many small UUID-like PKs can have very different memory footprints despite similar counts, so byte-size observability helps with capacity monitoring. UpsertUtils.estimatePrimaryKeyMapSizeInBytes samples up to 1000 keys to average PK content size (via PrimaryKey.asBytes() / ByteArray.length()), adds a per-entry overhead constant for the hashmap node + RecordLocation object, and extrapolates over the full key count, keeping the cost bounded regardless of map size. Computed at the same segment-lifecycle cadence as the existing count gauge (add/replace/remove/preload/close), not on the per-record ingest path. Wired into both ConcurrentMapPartitionUpsertMetadataManager and ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes via a new abstract getPrimaryKeyMapSizeInBytes() hook on BasePartitionUpsertMetadataManager.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18980 +/- ##
============================================
+ Coverage 65.05% 65.07% +0.01%
Complexity 1403 1403
============================================
Files 3399 3407 +8
Lines 212808 213139 +331
Branches 33568 33612 +44
============================================
+ Hits 138443 138699 +256
- Misses 63223 63269 +46
- Partials 11142 11171 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal upsert hot-path issue; see inline comment.
|
|
||
| protected void updatePrimaryKeyGauge() { | ||
| updatePrimaryKeyGauge(getNumPrimaryKeys()); | ||
| updatePrimaryKeyGauge(getNumPrimaryKeys(), getPrimaryKeyMapSizeInBytes()); |
There was a problem hiding this comment.
updatePrimaryKeyGauge() is still called from the realtime doAddRecord() paths, so this now runs getPrimaryKeyMapSizeInBytes() for every ingested upsert record. That samples up to 1000 keys and calls PrimaryKey.asBytes() while Pinot is on the per-record ingestion path, adding bounded but repeated map iteration/allocation to a hot path. Please keep the per-record update count-only and refresh the byte-size gauge from segment lifecycle, a rate-limited path, or background sampling instead.
There was a problem hiding this comment.
thanks.
split the per-record path onto a count-only overload; segment-lifecycle events (add/preload/replace/remove/close) and the periodic TTL eviction path still refresh both gauges.
updatePrimaryKeyGauge() was recomputing the byte-size gauge (bounded sample over the map, but still per-record overhead) on every ingested record via doAddRecord(), despite the intent of computing it only at segment-lifecycle cadence. Split it into a count-only overload used by the per-record hot path; segment lifecycle events (add/preload/replace /remove/close) and the periodic TTL eviction path continue to refresh both gauges.
3 participants
Add UPSERT_PRIMARY_KEY_MAP_SIZE_IN_BYTES ServerGauge to complement the existing key-count gauge. Tables with few large-string PKs and tables with many small UUID-like PKs can have very different memory footprints despite similar counts, so byte-size observability helps with capacity monitoring.
UpsertUtils.estimatePrimaryKeyMapSizeInBytes samples up to 1000 keys to average PK content size (via PrimaryKey.asBytes() / ByteArray.length()), adds a per-entry overhead constant for the hashmap node + RecordLocation object, and extrapolates over the full key count, keeping the cost bounded regardless of map size. Computed at the same segment-lifecycle cadence as the existing count gauge (add/replace/remove/preload/close), not on the per-record ingest path.
Wired into both ConcurrentMapPartitionUpsertMetadataManager and ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes via a new abstract getPrimaryKeyMapSizeInBytes() hook on BasePartitionUpsertMetadataManager.