devmadhuu commented on PR #9472: URL: https://github.com/apache/ozone/pull/9472#issuecomment-6398801808
> @devmadhuu , some questions: > > * It seems that the long description in the PR are mostly invalid. It particular "storing a 2-byte health state" and all the "memory efficient" calculations are invalid since the ContainerInfo is using the enum directly but not a short. If yes, could you revise the description? > > > ```java > // ContainerInfo > private ContainerHealthState healthState; > ``` > > * Also, the ContainerInfo.value actually is unused -- it is only used in tests. If yes, could we remove it? > > * BTW, how the value is assigned? > > * Define `ContainerHealthState.HEALTHY`. The description "Container is healthy" does not explain what does it mean. > > * Honestly, I also don't know the definitions for the other enums. The description field is unclear about the exact meaning. We need precise definitions. @sumitagrawl , do you know the precise definitions? > > * UNDER_REPLICATED as an example, what are the difference between the cases below? > > 1. UNDER_REPLICATED > 2. UNHEALTHY_UNDER_REPLICATED > 3. MISSING_UNDER_REPLICATED > 4. QUASI_CLOSED_STUCK_UNDER_REPLICATED > > * Why there are QUASI_CLOSED_STUCK_... enums but not QUASI_CLOSED without STUCK? > > * What was the goal for this PR? For metric and UI reports? Anything else? > > > Cc @umamaheswararao @szetszwo 1. You are right. The description reflects an earlier version that stored the health state as a short. I changed that to an enum reference following the earlier review, but missed updating the description. The merged code stores `ContainerHealthState` directly, so the two-byte-per-container claim and the memory calculations based on it are not in line per code. I have updated the description and remove those calculations. 2. Yes, you are right, the numeric value belongs to `ContainerHealthState` and was left over from the earlier short-based implementation. I found no public API or callers of its numeric getter or conversion method in the checked code; the callers are tests. The numbers are assigned explicitly in the enum declarations: 0–9 for individual states and 100–105 for combinations. They are not calculated from the container’s condition. The checks select an enum constant directly. I propose a small follow-up PR to remove the numeric field, lookup map, conversion methods, and tests that only check those numbers. The enum names and metric names would remain. 3. I agree that the descriptions for various container health and states need to be more precise. In the merged implementation, each container check starts with `HEALTHY`. If a check records another condition, that replaces it. Otherwise, the saved value remains HEALTHY. It is also the default when `ContainerInfo` is created, before `ReplicationManager` checks it. Some checks stop processing early—for example, the closing-container check—so this value does not guarantee that every replication and placement check has passed. I propose a follow-up PR to document the actual conditions for each state. We should agree on any stronger meaning for `HEALTHY` before changing its behavior and I think this would have had long history which @sodonnel might know better. As per my best understanding on meaning of below states for example: **`UNDER_REPLICATED`** - a Ratis container requiring three copies has only two healthy copies. **`UNHEALTHY_UNDER_REPLICATED`** - More copies are needed in a situation involving unhealthy replicas. One example is a Ratis container with no healthy copies and fewer unhealthy copies than required. **`MISSING_UNDER_REPLICATED`** - An EC container lacks enough distinct indexes to recover. A particularly useful example: EC MISSING does not necessarily mean zero replicas. For EC 3+2, retaining only indexes 1 and 2 is insufficient to reconstruct the data. If index 2 is on a decommissioning node without an in-service copy, the combination can be `MISSING_UNDER_REPLICATED`. `**QUASI_CLOSED_STUCK_UNDER_REPLICATED**` - A container cannot safely finish closing, and the check for stuck quasi-closed containers also finds too few copies of the remaining original replicas. 4. The main goal was to retain `ReplicationManager’s` health result on each in-memory ContainerInfo, so that same can be used by admin CLI for full container report, metrics and reports use the enum too, but the future goal also was to include supporting queries by health state. After this PR, a health-filtered container ID listing in SCM through [PR #10736 (https://github.com/apache/ozone/commit/575fe4f1cd7bdd9f6226051fcf7c260d4a24e1c8)](https://github.com/apache/ozone/commit/575fe4f1cd7bdd9f6226051fcf7c260d4a24e1c8), was done. -- 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]
