devmadhuu commented on PR #11199:
URL: https://github.com/apache/ozone/pull/11199#issuecomment-5740367743
> The other PR has been merged removing the transient state from
ReplicationManagerReport, so this PR can be updated based on current master.
>
> If we are going to report BCSID mis-match, then I would be inclined to
treat it like a first class health state like the others - under, over,
mis-replicated, unhealthy, QC, QC-Struck, BCSID-mismatch. This would simplify
the code a bit and also keep us with the "each container can only have one
state at a time" rule.
>
> @siddhantsangwan had a lot of experience with RM and @devmadhuu has worked
on lot of the Recon state reporting. What do you guys think?
+1 to treating mismatch as a first-class health state alongside the
existing ones, rather than a parallel dimension. `ContainerInfo.healthState` is
already a single enum, and every consumer — Recon's bucketing, `ozone admin
container info`, metrics, future automation — is built around that. Introducing
an another flag/dimension would force every one of those consumers to check
that, and anywhere if that misses, it silently drops the mismatch.
To address @F64116045 concern that mismatch shouldn't be hidden while other
issues are being repaired, we can follow the same pattern the enum already uses
for co-occurring conditions (e.g. QUASI_CLOSED_STUCK_UNDER_REPLICATED,
UNHEALTHY_UNDER_REPLICATED) and add named combined values for covering that
pairings as we do currently.:
```
- DATA_CHECKSUM_MISMATCH (baseline — no other problem)
- DATA_CHECKSUM_MISMATCH_UNDER_REPLICATED
- DATA_CHECKSUM_MISMATCH_OVER_REPLICATED
```
Also @sodonnel earlier point that the CLI/proto/RPC changes should be split
into a follow-up PR so this one can land the RM detection + report changes
cleanly first. Better break into multiple PRs for easy review.
**On Recon:** I think, making mismatch first-class also removes a whole
parallel detection path this PR currently adds in Recon. Right now
`ReconReplicationManager.processAll() `does its normal `processContainer(...,
readOnly=true) `pass and then it runs very recon specific -
`hasDataChecksumMismatch(container, replicas) `check, feeding matches into a
new `ReconReplicationManagerReport.replicaMismatchContainers `list. The Javadoc
also for `replicaMismatchContainers` field says that it has to be tracked
separately. Once mismatch is a real `ContainerHealthState` value, SCM's
`DataChecksumMismatchCheckHandler` runs during Recon's read-only pass exactly
like every other handler, stamps the state onto `ContainerInfo.healthState`,
and Recon's downstream bucketing reads it back the same way it reads `MISSING`,
`UNDER_REPLICATED`, etc. So `hasDataChecksumMismatch`,
`replicaMismatchContainers`, and `addReplicaMismatchContainer`, all can be
removed. Recon has one detection lane instead of
two. Simplify the code.
--
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]
