Count primary keys not replaced at removal, not from a stale bitmap clone - #19504
Open
KKcorps wants to merge 4 commits into
Open
Count primary keys not replaced at removal, not from a stale bitmap clone#19504KKcorps wants to merge 4 commits into
KKcorps wants to merge 4 commits into
Conversation
…lone The "Found N primary keys not replaced" warning and the upsertInconsistentRows/partialUpsertKeysNotReplaced metric are both computed from a validDocIds bitmap that was cloned before segment replacement started. Records for those keys can arrive while the replacement runs. Ingestion correctly moves them onto the newer consuming segment, but the replacement is still reading the pre-replacement clone, so keys that were replaced perfectly well are reported as not replaced. The metric is a false positive and the alert built on it is muted in production as a result. Count at the action boundary instead. removeSegment already decides, per key, whether the key still belongs to the segment being removed, inside computeIfPresent under a recordLocation.getSegment() == segment check. That is the authoritative answer. Return how many keys passed that check and report only those, so a key ingestion already moved is never counted. BasePartitionUpsertMetadataManager gains removeSegmentAndGetNumKeysRemoved(IndexSegment, MutableRoaringBitmap), which by default delegates to removeSegment and returns the bitmap cardinality, keeping existing metadata-manager implementations behaving exactly as before. Both ConcurrentMap managers override it and count real removals. The warning and the metric now only fire when the count is above zero. For the consistent-deletes manager the reporting also moves after doRemoveSegment, because that path walks every doc in the segment rather than the valid ones, so the removal itself is where the count becomes knowable. Postmortem: ZD#7481, RCA-299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyuTYJvV4ARAXR9wSXdjBi
KKcorps
force-pushed
the
kk/data-3101-upsert-metric-action-boundary
branch
from
September 8, 2026 05:44
36637a5 to
444c9bd
Compare
The count alone tells you a table is inconsistent but not which rows, so the next step is always a hunt. Keep the first few primary keys that fail the ownership check and log them. Free when nobody is looking. The list is not allocated unless the manager's logger has DEBUG enabled, so the steady state is one null check per removed key. The keys themselves cost nothing to obtain: they are already in hand at the increment, and UpsertUtils.getPrimaryKeyIterator hands out a fresh PrimaryKey per doc (PrimaryKeyReader.getPrimaryKey(int) allocates), so holding a reference is safe and needs no copy. Bounded at 8 keys per removal. DEBUG rather than WARN on purpose. A primary key is customer data, and every other log line in this package reports key counts rather than key values. The count stays at WARN where it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyuTYJvV4ARAXR9wSXdjBi
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19504 +/- ##
============================================
+ Coverage 67.72% 67.73% +0.01%
- Complexity 1430 1450 +20
============================================
Files 3489 3490 +1
Lines 224634 225015 +381
Branches 35468 35523 +55
============================================
+ Hits 152130 152412 +282
- Misses 60485 60560 +75
- Partials 12019 12043 +24
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:
|
…e DEBUG line Follow-up on the DEBUG sample: put the keys in the warning that already reports the count, so one line carries both. Found 2 primary keys not replaced for segment: X, first 2: [[200], [300]] Flipping the DEBUG line to WARN in place would have been wrong. removeSegmentAndGetNumKeysRemoved is also on the plain segment-removal path, where removing every key the segment owns is normal, so a warning there would fire on every retention deletion and name customer keys for a healthy operation. Instead the sample is collected into a caller-supplied list. The replacement path passes one and reports it; the plain removal path passes null and collects nothing, so it neither samples nor logs. The base default cannot tell which candidates were actually removed, so it names no keys and logKeysNotReplaced falls back to the count-only message. The isDebugEnabled gate is gone with the DEBUG line. The cost is now one null check per removed key on the plain path, and one bounded ArrayList per replacement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyuTYJvV4ARAXR9wSXdjBi
KKcorps
marked this pull request as ready for review
September 8, 2026 09:13
deepthi912
reviewed
Sep 11, 2026
| removeSegment(oldSegment, validDocIdsForOldSegment); | ||
| int numKeysStillNotReplaced = | ||
| removeSegmentAndGetNumKeysRemoved(oldSegment, validDocIdsForOldSegment); | ||
| if (numKeysStillNotReplaced > 0) { |
Collaborator
There was a problem hiding this comment.
How would this give us information about the keys that are not replaced? validDocIdsForOldSegment will be gone down to 0 here if shouldRevertMetadataOnInconsistency is false right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix false “primary keys not replaced” warnings and inconsistent-row metrics during segment replacement. A key that ingestion moved to a newer segment can remain in the old
validDocIdsclone; reporting the clone's cardinality incorrectly counts that key as unreplaced.Add
removeSegmentAndGetNumKeysRemoved(IndexSegment, MutableRoaringBitmap)and count only keys that still belong to the old segment at the existing per-key removal check. Both ConcurrentMap managers use the corrected count. The base implementation preserves existing behavior for other metadata managers.This PR only corrects the count and suppresses reports when it is zero. The existing warning text, metric names, and configuration remain unchanged. No primary-key samples are collected or logged.
Validation
BasePartitionUpsertMetadataManagerTest,ConcurrentMapPartitionUpsertMetadataManagerTest, andConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletesTest.Related change
StarTree RocksDB support, including asynchronous counting and bounded stored-key samples, is in https://github.com/startreedata/startree-pinot/pull/4260.
Postmortem: ZD#7481 / RCA-299. Unmute the affected alert only after every server for the table runs the corrected implementation.