suvodeep-pyne opened a new pull request, #19909:
URL: https://github.com/apache/pinot/pull/19909
## Summary
On pauseless tables with dedup or partial upsert,
`PinotLLCRealtimeSegmentManager.repairSegmentsInErrorState` returns early and
repairs no ERROR segment at all. That includes segments whose other replica is
ONLINE, where a reset only reloads data a peer or the deep store already holds.
This PR keeps the dedup / partial-upsert guard on re-ingestion only, resets
just the ERROR replicas, and makes the skipped repairs visible.
## Motivation
`RealtimeSegmentValidationManager` runs this repair on every pauseless
table. #16811 added an early return that skips all of it when
`allowRepairOfCommittingSegments` is false, which is the default for dedup and
partial-upsert tables:
```java
if (isPauselessTable && !repairCommittingSegments) {
// We do not run reingestion for dedup and partial upsert tables in
pauseless as it can
// lead to data inconsistencies
...
return;
}
```
The comment names the actual risk, re-ingestion, but the return also skips
the reset branches. A reset takes an ERROR replica through OFFLINE and back, so
it downloads the committed segment from the deep store or from a peer. It never
re-consumes, so it cannot add duplicate or inconsistent rows. Before #16811,
those resets ran on pauseless dedup tables by default, because
`controller.segment.error.autoReset` defaults to true. Non-pauseless dedup
tables still run them.
On a dedup or partial-upsert table, one unrepaired replica can stop the
server consuming. The server's consumers wait until every segment the
IdealState marks ONLINE for that server is ONLINE in its current state
(`TableStateUtils.isAllSegmentsLoaded`). One ERROR replica keeps that check
false, so the server stops consuming the table. Each later segment then fails
to go ONLINE on that server too, and the repair never helps. Seen on a
2-replica pauseless dedup table: 5 segments were ONLINE on one server and ERROR
on the other. Every 15-minute validation run logged the same two lines and did
nothing:
```
Found 5 segments with at least one replica in ERROR state: [...], 0 segments
with all replicas in ERROR state: [] in table: ..._REALTIME, repairing them
Skipping repair for errored segments in table: ..._REALTIME because dedup is
enabled
```
Two related problems in the same method:
- **The reset is not targeted.** `resetSegment(table, segment, null)` resets
every replica in the IdealState, including the healthy ones. That can briefly
leave a partition with zero ONLINE replicas. On a dedup or upsert table it also
drops the healthy replica's keys from the metadata manager for a moment while
other segments keep consuming. If the segment has no deep-store copy, it
bounces the only good copy.
- **The skip is not visible.** In the early-return path,
`pauselessSegmentsInErrorCount` is never set and
`pauselessSegmentsInUnrecoverableErrorCount` counts only segments with every
replica in ERROR. Both read 0 in the case above.
## Changes
- **Gate only re-ingestion.** The early return is removed. Re-ingestion of a
COMMITTING segment with all replicas in ERROR stays gated on
`allowRepairOfCommittingSegments`, which is the
`repairErrorSegmentsForPartialUpsertOrDedup` task property or
`disasterRecoveryMode=ALWAYS`. The reset branches now run for every table.
- **Reset only the ERROR replicas.** `resetSegment` is now called once per
replica that is in ERROR in the external view, not once per segment with no
target. If one reset fails, the failure is logged and repair continues with the
other segments. Before, one failure threw and stopped the whole pass.
- **Make skipped repairs visible.**
- Each skipped COMMITTING segment now logs a WARN that says how to enable
the repair.
- The `allowRepairOfCommittingSegments` log lines now say that only
re-ingestion is disallowed.
- `pauselessSegmentsInErrorCount` (and `segmentsInError` on non-pauseless
tables) now counts segments with **any** ERROR replica, not only segments with
every replica in ERROR.
- `pauselessSegmentsInUnrecoverableErrorCount` counts only segments that
this pass actually leaves unrepaired.
### Behavior change for reviewers
- **Resets now run on pauseless dedup and partial-upsert tables without an
override.** These are the same resets that pauseless tables without dedup, and
non-pauseless dedup tables, already get. One of them resets an IN_PROGRESS
segment with every replica in ERROR, which restarts consumption from the
segment's start offset. That is what an operator does by hand today.
- **`segmentsInError` now agrees more closely with `SegmentStatusChecker`.**
For non-pauseless tables, `SegmentStatusChecker` also writes this gauge, as the
number of ERROR replicas. `repairSegmentsInErrorState` used to overwrite it
with the number of segments with every replica in ERROR, so the gauge often
dropped to 0 between status-checker runs. The two counts are now equal whenever
each errored segment has one ERROR replica.
## Testing
- **New integration test `PauselessDedupErrorReplicaRepairTest`**
- Setup: a 2-replica pauseless dedup table with **no** disaster-recovery
override. Consuming-transition failures are injected on one server only, so
every affected segment keeps one healthy replica. The test runs the
segment-level validation and requires every ERROR replica to recover and the
deduplicated count to be served.
- On master it fails: the controller logs `Skipping repair for errored
segments ... because dedup is enabled` on every pass.
- With this change it passes.
- **Existing `PauselessDedupRealtimeIngestionSegmentCommitFailureTest` still
passes.** It runs with the override and covers commit failures and
consuming-transition failures, so it shows the targeted resets and the
re-ingestion path still recover fully.
- **`PinotLLCRealtimeSegmentManagerTest` has two new unit tests:**
- On a pauseless dedup table, the ERROR replicas of COMMITTING, DONE and
IN_PROGRESS segments are each reset individually, and healthy replicas are
never reset. The gauges read 4 segments with an ERROR replica and 1 left
unrepaired. On master this test fails because no reset is issued.
- With the override, the re-ingestion path is taken.
- `PauselessRealtimeIngestionSegmentCommitFailureTest` (no dedup) and
`RealtimeSegmentValidationManagerTest` still pass. Spotless, checkstyle and
license checks pass on `pinot-controller` and `pinot-integration-tests`.
--
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]
