You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/08/09 18:10:15 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #3667: HDDS-7092. : EC: Offline Recovery with simultaneous Over Replication …
swamirishi opened a new pull request, #3667:
URL: https://github.com/apache/ozone/pull/3667
…& 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
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3667:
URL: https://github.com/apache/ozone/pull/3667#issuecomment-1223934341
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?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3667:
URL: https://github.com/apache/ozone/pull/3667#issuecomment-1324760043
Fixed with an alternative approach in [#3984](https://github.com/apache/ozone/pull/3984)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel closed pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
Posted by GitBox <gi...@apache.org>.
sodonnel closed pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
URL: https://github.com/apache/ozone/pull/3667
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a diff in pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3667:
URL: https://github.com/apache/ozone/pull/3667#discussion_r947503736
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECOverReplicationHandler.java:
##########
@@ -74,23 +75,26 @@ public Map<DatanodeDetails, SCMCommand<?>> processAndCreateCommands(
Set<ContainerReplica> replicas, List<ContainerReplicaOp> pendingOps,
ContainerHealthResult result, int remainingMaintenanceRedundancy) {
ContainerInfo container = result.getContainerInfo();
- ContainerHealthResult currentUnderRepRes = ecContainerHealthCheck
+ Optional<ContainerHealthResult> currentUnderRepRes = ecContainerHealthCheck
.checkHealth(container, replicas, pendingOps,
- remainingMaintenanceRedundancy);
+ remainingMaintenanceRedundancy,
+ Optional.of(ContainerHealthResult.HealthState.OVER_REPLICATED));
LOG.debug("Handling over-replicated EC container: {}", container);
//sanity check
- if (currentUnderRepRes.getHealthState() !=
- ContainerHealthResult.HealthState.OVER_REPLICATED) {
+ if (currentUnderRepRes.map(containerHealthResult ->
+ containerHealthResult.getHealthState()
+ != ContainerHealthResult.HealthState.OVER_REPLICATED)
+ .orElse(true)) {
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: {}",
Review Comment:
Are there tabs here?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECContainerReplicaCount.java:
##########
@@ -342,6 +342,10 @@ public List<Integer> overReplicatedIndexes(boolean includePendingDelete) {
return indexes;
}
Review 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?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECContainerHealthCheck.java:
##########
@@ -67,20 +83,23 @@ public ContainerHealthResult checkHealth(ContainerInfo container,
dueToDecommission = false;
remainingRedundancy = repConfig.getParity() - missingIndexes.size();
}
- return new ContainerHealthResult.UnderReplicatedHealthResult(
- container, remainingRedundancy, dueToDecommission,
- replicaCount.isSufficientlyReplicated(true),
- replicaCount.isUnrecoverable());
+ containerHealthResults.add(
+ new ContainerHealthResult.UnderReplicatedHealthResult(
+ container, remainingRedundancy, dueToDecommission,
+ replicaCount.isSufficientlyReplicated(true),
+ replicaCount.isUnrecoverable()));
}
if (replicaCount.isOverReplicated(false)) {
List<Integer> overRepIndexes = replicaCount.overReplicatedIndexes(false);
- return new ContainerHealthResult
+ containerHealthResults.add(new ContainerHealthResult
.OverReplicatedHealthResult(container, overRepIndexes.size(),
- !replicaCount.isOverReplicated(true));
+ !replicaCount.isOverReplicated(true)));
}
- // No issues detected, so return healthy.
- return new ContainerHealthResult.HealthyResult(container);
+ // If No issues detected, return healthy.
Review Comment:
Nit typo: No -> no
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3667:
URL: https://github.com/apache/ozone/pull/3667#issuecomment-1222461718
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:
1. What if we return over-replication health first, before under-replication? That way, we can fix this with a small change, by just changing the order in the ECContainerHealthCheck. The negative is that we have to wait longer to fix an under-replication, but the change here will be small and the general flow stays the same.
2. Or, inside the under-replication handler, we have access to the methods in ECContainerReplicaCount which can tell us if there are over-replicated indexes. If, and only if, we get a failure in the UnderRep handler due to not finding enough nodes, we can somehow switch to over-replication handling and remove the excess replicas. However that starts to muddle the over and under replication logic together, which I was trying to avoid.
I probably need to think about it a bit more.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on pull request #3667: HDDS-7092. EC Offline Recovery with simultaneous Over Replication
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3667:
URL: https://github.com/apache/ozone/pull/3667#issuecomment-1222642747
> 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.
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 think delaying under replication is not a good idea. or should be check cluster size and over replications are at same size then only we return over replication first? I know this would introduce one more ugly check though.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org