sadanand48 commented on code in PR #9964:
URL: https://github.com/apache/ozone/pull/9964#discussion_r2979459294
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -199,47 +199,92 @@ public boolean shouldBeExcluded(ContainerID containerID,
* Checks whether specified container is closed. Also checks if the replica
* on the specified datanode is CLOSED. Assumes that there will only be one
* replica of a container on a particular Datanode.
+ * <p>
+ * With hdds.container.balancer.include.non.standard.containers set to true:
+ * - CLOSED containers: Allows moving QUASI_CLOSED replicas if minimum
CLOSED replicas exist
+ * - QUASI_CLOSED containers: Allows moving if all replicas are QUASI_CLOSED
+ *
* @param container container to check
- * @param datanodeDetails datanode on which a replica of the container is
- * present
- * @return true if container LifeCycleState is
- * {@link HddsProtos.LifeCycleState#CLOSED} and its replica on the
- * specified datanode is CLOSED, else false
+ * @param datanodeDetails datanode on which a replica of the container is
present
+ * @param replicas all replicas of the container
+ * @return true if container and replica are eligible for balancing, else
false
*/
private boolean isContainerClosed(ContainerInfo container,
DatanodeDetails datanodeDetails,
Set<ContainerReplica> replicas) {
- if (!container.getState().equals(HddsProtos.LifeCycleState.CLOSED)) {
+ HddsProtos.LifeCycleState containerState = container.getState();
+ // Find the specific replica on this datanode
+ ContainerReplica targetReplica = replicas.stream()
+ .filter(r -> r.getDatanodeDetails().equals(datanodeDetails))
+ .findFirst()
+ .orElse(null);
+ if (targetReplica == null) {
return false;
}
-
- for (ContainerReplica replica : replicas) {
- if (replica.getDatanodeDetails().equals(datanodeDetails)) {
- // don't consider replica if it's not closed
- // assumption: there's only one replica of this container on this DN
- return replica.getState().equals(ContainerReplicaProto.State.CLOSED);
+ ContainerReplicaProto.State replicaState = targetReplica.getState();
+
+ // Case 1: Container is CLOSED
+ if (containerState == HddsProtos.LifeCycleState.CLOSED) {
+ if (replicaState == ContainerReplicaProto.State.CLOSED) {
+ return true;
}
+
+ // With config enabled: Also allow QUASI_CLOSED replicas if we have
minimum required CLOSED replicas
+ if (balancerConfiguration.getIncludeNonStandardContainers()) {
+ long numClosedReplicas = replicas.stream()
+ .filter(r -> r.getState() == ContainerReplicaProto.State.CLOSED)
+ .count();
+ int minRequiredReplicas =
container.getReplicationConfig().getRequiredNodes();
+
+ if (numClosedReplicas >= minRequiredReplicas) {
+ return replicaState == ContainerReplicaProto.State.QUASI_CLOSED;
+ }
+ }
+ return false;
+ }
+
+ // Case 2: Container is QUASI_CLOSED
+ if (containerState == HddsProtos.LifeCycleState.QUASI_CLOSED) {
+ if (!balancerConfiguration.getIncludeNonStandardContainers()) {
+ return false;
+ }
+
+ // Only allow if ALL replicas are QUASI_CLOSED
+ // This ensures we don't move containers with mixed replica states
+ return replicas.stream()
+ .allMatch(r -> r.getState() ==
ContainerReplicaProto.State.QUASI_CLOSED);
}
-
return false;
}
/**
* This asks replication manager whether a container is under/over/mis
replicated. The intention is the same as
* isContainerReplicatingOrDeleting but the check is done in a different way
to be doubly sure.
+ * <p>
+ * With hdds.container.balancer.include.non.standard.containers set to true:
+ * - OVER_REPLICATED containers are allowed
+ *
* @param container container to check
* @param replicas the container's replicas
* @return false if it should not be moved, true otherwise
*/
private boolean isContainerHealthyForMove(ContainerInfo container,
Set<ContainerReplica> replicas) {
ContainerHealthResult.HealthState state =
replicationManager.getContainerReplicationHealth(container,
replicas).getHealthState();
- if (state != ContainerHealthResult.HealthState.HEALTHY) {
- LOG.debug("Excluding container {} with replicas {} as its health is
{}.", container, replicas, state);
- return false;
+ if (state == ContainerHealthResult.HealthState.HEALTHY) {
+ return true;
Review Comment:
same here
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -199,47 +199,92 @@ public boolean shouldBeExcluded(ContainerID containerID,
* Checks whether specified container is closed. Also checks if the replica
* on the specified datanode is CLOSED. Assumes that there will only be one
* replica of a container on a particular Datanode.
+ * <p>
+ * With hdds.container.balancer.include.non.standard.containers set to true:
+ * - CLOSED containers: Allows moving QUASI_CLOSED replicas if minimum
CLOSED replicas exist
+ * - QUASI_CLOSED containers: Allows moving if all replicas are QUASI_CLOSED
+ *
* @param container container to check
- * @param datanodeDetails datanode on which a replica of the container is
- * present
- * @return true if container LifeCycleState is
- * {@link HddsProtos.LifeCycleState#CLOSED} and its replica on the
- * specified datanode is CLOSED, else false
+ * @param datanodeDetails datanode on which a replica of the container is
present
+ * @param replicas all replicas of the container
+ * @return true if container and replica are eligible for balancing, else
false
*/
private boolean isContainerClosed(ContainerInfo container,
Review Comment:
Lets have a separate method isContainerClosedRelaxed and use that if the
config is true.
--
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]
