KKcorps opened a new pull request, #19499:
URL: https://github.com/apache/pinot/pull/19499
## TL;DR
`UpsertCompactionTask`'s generator already compares valid doc counts across
replicas and refuses
to schedule a segment whose replicas disagree. Today that refusal is a log
line and nothing else,
so the controller notices that two replicas of an upsert table hold
different data, quietly stops
compacting the segment, and tells no one. This PR emits a meter for it,
scoped to the one skip
reason that actually means divergence.
## The problem
`MinionTaskUtils.selectValidDocIdsMetadataForConsensus` is the generator's
pre-scheduling gate. It
returns `null` to skip a segment, and it does that for six different
reasons. Five of them are
"the answer is not trustworthy right now". One of them means the replicas
have genuinely drifted
apart: in `EQUAL` mode, two replicas report a different number of valid docs
for the same segment
with the same CRC, both servers healthy, all replicas accounted for.
That last case is the interesting one and it is invisible. There is no
metric on it, so a table can
sit with diverged replicas for weeks, serving different answers depending on
which replica the
broker picks, while compaction silently stalls on the affected segments.
```mermaid
flowchart LR
subgraph Before["❌ Today"]
A[replicas report<br/>different valid doc counts] --> B[generator skips
segment]
B --> C[LOGGER.warn]
C --> D[nobody knows<br/>compaction stalls]
end
subgraph After["✅ With this PR"]
E[replicas report<br/>different valid doc counts] --> F[generator skips
segment]
F --> G[LOGGER.warn names<br/>both servers and both counts]
F --> H[UpsertCompactionSegmentsSkipped<br/>meter per table]
H --> I[alertable]
end
```
## The approach
1. Add
`ControllerMeter.UPSERT_COMPACTION_SEGMENT_SKIPPED_CONSENSUS_FAILURE`, a
per-table meter
exported as `UpsertCompactionSegmentsSkipped`.
2. Give `selectValidDocIdsMetadataForConsensus` an optional
`ControllerMetrics` and the table name,
and bump that meter from the `EQUAL`-mode disagreement branch only.
3. Thread `ControllerMetrics` and `tableNameWithType` from
`UpsertCompactionTaskGenerator`'s
`generateTasks` into `processValidDocIdsMetadata`, which is where the
per-segment loop lives.
4. Keep the existing five-argument `selectValidDocIdsMetadataForConsensus`
as an overload that
meters nothing, so the other caller is untouched.
5. Extend the disagreement log line to name both servers and both counts.
## Which skip reasons are metered
Only the last row. The other five are transient states that resolve on their
own, and metering them
would make the signal fire during every segment reload and every rolling
restart.
| Why the generator skips the segment | Metered | Why not |
|---|---|---|
| No replica reported metadata | no | Nothing to compare yet |
| Replica's CRC will not parse | no | Bad response, not bad data |
| Replica CRC does not match ZK | no | Usually a reload in flight |
| A server is not in `GOOD` state | no | It may still be mutating the
segment |
| Fewer replicas responded than expected | no | Cannot confirm consensus
either way |
| `EQUAL` mode, replicas report different valid doc counts | **yes** | The
replicas hold different data |
This is also why the meter sits inside the helper rather than at the
generator's
`if (validDocIdsMetadata == null) { continue; }`. By the time the caller
sees the `null`, the reason
is gone.
## Flow
```mermaid
sequenceDiagram
participant G as UpsertCompactionTaskGenerator
participant R as ServerSegmentMetadataReader
participant S as Servers
participant U as MinionTaskUtils
participant M as ControllerMetrics
G->>R: getSegmentToValidDocIdsMetadataFromServer
R->>S: validDocIdsMetadata per segment batch
S-->>R: per-replica valid doc counts, CRCs, server status
R-->>G: segment -> List<ValidDocIdsMetadataInfo>
loop per candidate segment
G->>U: selectValidDocIdsMetadataForConsensus(..., metrics,
tableNameWithType)
alt CRC mismatch, unhealthy server, or short responder list
U-->>G: null (skipped, not metered)
else EQUAL mode and counts differ
U->>M: addMeteredTableValue(UpsertCompactionSegmentsSkipped, 1)
U-->>G: null (skipped, metered)
else replicas agree
U-->>G: chosen replica
end
end
```
## New metrics
| Metric | Type | Meaning |
|---|---|---|
| `UPSERT_COMPACTION_SEGMENT_SKIPPED_CONSENSUS_FAILURE`
(`UpsertCompactionSegmentsSkipped`) | Meter, per table | A segment was not
scheduled for `UpsertCompactionTask` because its replicas reported different
valid doc counts |
No configuration changes. No JMX-to-Prometheus exporter changes either:
`controller.yml` and
`pinot.yml` both end in a catch-all pattern for table-scoped meters, so this
exports as
`pinot_controller_UpsertCompactionSegmentsSkipped_Count` with `table` and
`tableType` labels.
`YammerControllerPrometheusMetricsTest` enumerates every `ControllerMeter`
and asserts each one
exports correctly, and it passes with the new entry, so that name and label
set is verified rather
than assumed.
## Compatibility notes
- `UpsertCompactMergeTask` is deliberately untouched. It keeps calling the
five-argument
`selectValidDocIdsMetadataForConsensus` overload, which meters nothing,
and no file under
`upsertcompactmerge/` is in this diff.
- The existing, declared-but-unused
`ControllerMeter.UPSERT_COMPACT_MERGE_SEGMENT_SKIPPED_CONSENSUS_FAILURE`
is left exactly as it is.
This PR does not wire it. Its name is specific to the compact-merge task,
so reusing it for the
compaction task would have exported a misleading metric name.
- `UpsertCompactionTaskGenerator.processValidDocIdsMetadata` is
`@VisibleForTesting public static`
and gains two parameters: `tableNameWithType` first and a nullable
`ControllerMetrics` last. Any
out-of-tree caller of that method needs updating. Passing `null` for the
metrics keeps the old
behavior exactly.
## Performance considerations
- Nothing new runs on the happy path. When the replicas agree, the added
code is not reached.
- On a disagreement the cost is one `addMeteredTableValue` call, which is a
map lookup plus a meter
mark, on a path that already just decided to skip work.
- The extended log line reads two more fields off objects the code already
holds, and only inside a
branch that was already logging.
## Testing
`UpsertCompactionTaskGeneratorTest.testProcessValidDocIdsMetadataConsensusFailureMeter`
drives
`processValidDocIdsMetadata` through each case and asserts the meter's delta:
| Case | Expected meter delta |
|---|---|
| Replicas agree | 0 |
| CRC mismatch between a replica and ZK | 0 |
| One server in `STARTING` | 0 |
| Only one of two replicas responded | 0 |
| Replicas report different valid doc counts, `EQUAL` mode | 1 |
| Same disagreement under `MOST_VALID_DOCS` | 0, the mode picks a winner
instead of skipping |
| Same disagreement with a `null` `ControllerMetrics` | 0, and no exception |
The existing `testProcessValidDocIdsMetadataConsensus` still covers the
selection behavior itself and
is unchanged apart from the two new arguments. The suite passes, as does
`YammerControllerPrometheusMetricsTest` for the new metric's export.
--
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]
