deepthi912 opened a new pull request, #19110: URL: https://github.com/apache/pinot/pull/19110
## Summary - Consolidates the `shouldRevertMetadataOnInconsistency` fork from three call sites (`replaceSegment`, `doRemoveSegment`, `ConsistentDeletes.replaceSegment`) into a single helper `removeSegmentWithReconciliation` on `BasePartitionUpsertMetadataManager`. Preserves every existing log line, metric emission, and side-effect on both the revert and plain-removal paths — the refactor is a strict behavior-preserving cleanup on those paths. - Adds an ERROR log line and `REALTIME_UPSERT_INCONSISTENT_ROWS` metric emission for **FULL upsert tables** (with or without `deleteRecordColumn`) when residual valid docs are found on the old segment during segment replacement. This closes an observability gap where FULL upsert configurations silently swallow this class of cross-replica drift because `isTableTypeInconsistentDuringConsumption()` gates the existing log/metric path. ## Motivation We've seen cross-replica `COUNT(*)` divergence on production FULL upsert tables with `deleteRecordColumn=<col>` where a residual valid-doc set remains on the old segment after `replaceSegment`. Because `isTableTypeInconsistentDuringConsumption()` returns false for these tables (partial upsert / dropOutOfOrder / outOfOrderRecordColumn are the only true branches), the WARN + metric that would normally surface this drift never fires. Operators had no signal for the condition until manual reload uncovered it — the drift can persist across restarts and compaction cycles undetected. This PR does not change the underlying `doAddRecord` / `doAddOrReplaceSegment` upsert semantics. It only reroutes the shared dispatch and lights up the observability path for the FULL branch. ## Behavior audit | Call site & condition | Before | After | Δ | |---|---|---|---| | `replaceSegment`, `isTableTypeInconsistent=true`, `shouldRevert=true` | INFO revert → `removeSegment(validDocIds)` (concrete forks to revert) → post-check | Same via helper | none | | `replaceSegment`, `isTableTypeInconsistent=true`, `shouldRevert=false` | `logInconsistentResults(card)` → plain `removeSegment` | Same via helper | none | | `replaceSegment`, FULL (`isTableTypeInconsistent=false`) | Silent plain removal | `logInconsistentResults(card)` (ERROR + `REALTIME_UPSERT_INCONSISTENT_ROWS`) → plain removal | **new signal, intended** | | `doRemoveSegment`, `shouldRevert=true` | `revertSegmentUpsertMetadata` → post-check | Same via helper | none | | `doRemoveSegment`, `shouldRevert=false` | Silent plain removal | Silent plain removal | none | | `ConsistentDeletes.replaceSegment`, `shouldRevert=true` | `revertSegmentUpsertMetadata` → early return | Same via helper → early return | none | | `ConsistentDeletes.replaceSegment`, `isTableTypeInconsistent=true`, `shouldRevert=false` | `logInconsistentResults(card)` → `doRemoveSegment` | Same | none | | `ConsistentDeletes.replaceSegment`, FULL | Silent `doRemoveSegment` | `logInconsistentResults(card)` → `doRemoveSegment` | **new signal, intended** | | `logInconsistentResults`, `dropOutOfOrderRecord` / `outOfOrderRecordColumn` | `REALTIME_UPSERT_INCONSISTENT_ROWS` | Same | none | | `logInconsistentResults`, partial upsert | `PARTIAL_UPSERT_KEYS_NOT_REPLACED` | Same | none | | `logInconsistentResults`, FULL | No metric | `REALTIME_UPSERT_INCONSISTENT_ROWS` (`else` branch) | **new signal, intended** | ## Metric naming note `REALTIME_UPSERT_INCONSISTENT_ROWS` is now emitted for both `dropOutOfOrderRecord`/`outOfOrderRecordColumn` tables (as before) and FULL upsert tables (new). Semantically it is now the general "keys not properly replaced during upsert segment replacement" counter. `PARTIAL_UPSERT_KEYS_NOT_REPLACED` semantics are unchanged — partial upsert continues to emit only the partial-specific counter. Existing dashboards that alert on `REALTIME_UPSERT_INCONSISTENT_ROWS` will start seeing signal from FULL-upsert tables, which is the intent. ## Test plan - [x] `./mvnw -pl pinot-segment-local -am -Dcheckstyle.skip=true compile` — clean - [ ] `./mvnw -pl pinot-segment-local -Dtest=BasePartitionUpsertMetadataManagerTest test` - [ ] `./mvnw -pl pinot-segment-local -Dtest=ConcurrentMapPartitionUpsertMetadataManagerTest test` - [ ] Manual verification with a FULL upsert + `deleteRecordColumn` table that the ERROR log fires and `REALTIME_UPSERT_INCONSISTENT_ROWS` increments on segment-replace with residual valid docs. -- 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]
