HDDS-7092. EC Offline Recovery with simultaneous Over Replication #3667
Conversation
swamirishi
changed the title
swamirishi
changed the title
swamirishi
changed the title
There was a problem hiding this comment.
Thanks @swamirishi for working on this JIRA. I have reviewed the patch.
| } | ||
|
|
||
| // No issues detected, so return healthy. | ||
| return new ContainerHealthResult.HealthyResult(container); | ||
| // If No issues detected, return healthy. |
There was a problem hiding this comment.
Nit typo: No -> no
| LOG.info("The container {} state changed and it's not in over" | ||
| + " replication any more. Current state is: {}", | ||
| container.getContainerID(), currentUnderRepRes); | ||
| + " replication any more. Current state is: {}", |
There was a problem hiding this comment.
Are there tabs here?
| @@ -342,6 +342,10 @@ public List<Integer> overReplicatedIndexes(boolean includePendingDelete) { | |||
| return indexes; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
right now you are using it only for size? why it is returning set. Is there any other plans to use? Just healthy index count may work?
|
I'm not sure about this change. I feel it adds quite a bit of complexity to the model to handle what is really an edge case for a small cluster. I'd like a container to be handled for only a single state at a time as we could get into a position where we try reconstruction only to have a replica we are reading from removed by over-replication handling at the same time. It also makes things easier to think about, rather than the container having many states. Few thoughts come to mind:
I probably need to think about it a bit more. |
I am not sure this patch added much complexity than what we have, but your pointed case is a good one to rethink on this patch. I agree this should be a corner case and this is possible only for smaller clusters. However we should fix this though as some test clusters can go into this situations and we should have some solution for it. |
|
I feel we should handle this in the under-replication handler. In normal circumstances, the container can be under-replicated and it might be over replicated too, but if we can create enough new replicas for under-replication we don't care about the over-replication at this stage. However if we find we cannot place enough new replicas, we can check for over-replication. If it is over replicated too, we can refactor the code so we can call into the over-replication handler and schedule the delete container commands. This would avoid removing replicas we may depend on for reconstruction or copy. It also avoids the race condition where we queue both an under and over replication, and the under-replication would fail until the over replication gets processed and completed. If we handle it in the under replication handler, the health check code still only returns a single state (healthy, under or over replicated). When we integrate the Ratis code, it is not possible for a container to be both under and over replicated, so it keeps the code consistent between the two flows, which is helpful to understanding in the future. What do you think? |
|
Fixed with an alternative approach in #3984 |
…& Under Replication
What changes were proposed in this pull request?
In case of overreplication & underreplication happening together, it could so happen that all nodes are excluded in case of underreplication. Thus only underreplicationHandler would continuously fail & overreplication will not run.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7092
How was this patch tested?
Unit Tests, Integration Tests