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 2021/08/25 17:08:36 UTC
[GitHub] [ozone] siddhantsangwan opened a new pull request #2578: HDDS-5671. ContainerBalancer#stop interrupts the balancing thread before letting it join.
siddhantsangwan opened a new pull request #2578:
URL: https://github.com/apache/ozone/pull/2578
## What changes were proposed in this pull request?
ContainerBalancer#stop interrupts the balancing thread before letting it join:
```
balancerRunning = false;
currentBalancingThread.interrupt();
currentBalancingThread.join(1000);
```
This causes:
> [ContainerBalancer] WARN balancer.ContainerBalancer (ContainerBalancer.java:stop(731)) - Interrupted while waiting for balancing thread to join.
> [ContainerBalancer] INFO balancer.ContainerBalancer (ContainerBalancer.java:stop(736)) - Container Balancer stopped successfully.
ContainerBalancer should instead let the balancing thread join before calling interrupt.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5671
## How was this patch tested?
TestContainerBalancer UT.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723188402
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
How about instead of calling lock(), we use tryLock()? If tryLock() returns false then the lock is either held in the start or the stop method. In both the cases, we can just return from the stop method.
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723165075
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
You will need to make sure that start does not succeed until join has completed for the older thread.
--
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] JacksonYao287 commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-938600805
IMHO, it is better off using a completeableFuture to run balancer asynchronously , not an os thread.
1 when we are going to start a balancer , if the completeableFuture `isDone()` or null , we can create a new completeableFuture to run balancer asynchronously.
2 when we want to check the balancer status , just return `completeableFuture.isDone() || completeableFuture == null`
3 if we want to stop balancer from command line when balancer is running , just call something like `cancel()` or else.
4 if completeableFuture completes, nothing to do.
this is the basic idea, please take a deep thought!
--
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] lokeshj1703 commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-946527433
@siddhantsangwan Thanks for the contribution! @JacksonYao287 Thanks for the review! I have committed the PR to master branch.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718451563
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -348,17 +361,24 @@ private boolean initializeIteration() {
}
private IterationResult doIteration() {
+ // note that potential and selected targets are updated in the following
+ // loop
List<DatanodeDetails> potentialTargets = getPotentialTargets();
Set<DatanodeDetails> selectedTargets =
new HashSet<>(potentialTargets.size());
moveSelectionToFutureMap = new HashMap<>(unBalancedNodes.size());
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning || Thread.currentThread().isInterrupted()) {
Review comment:
Yes, I think checking only `balancerRunning` is enough. I'll change this.
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-939744991
> IMHO, it is better off using a completeableFuture to run balancer asynchronously , not an os thread.
>
> 1 when we are going to start a balancer , if the completeableFuture `isDone()` or null , we can create a new completeableFuture to run balancer asynchronously.
>
> 2 when we want to check the balancer status , just return `completeableFuture.isDone() || completeableFuture == null`
>
> 3 if we want to stop balancer from command line when balancer is running , just call something like `cancel()` or else.
>
> 4 if completeableFuture completes, nothing to do.
>
> this is the basic idea, please take a deep thought!
@JacksonYao287 thanks for the suggestion! What do you think about this @lokeshj1703 ?
--
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] lokeshj1703 commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-931981353
@siddhantsangwan There are a few conflicts.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r730793878
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -754,17 +773,21 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
-
- // allow garbage collector to collect balancing thread
- currentBalancingThread = null;
- } catch (InterruptedException e) {
- LOG.warn("Interrupted while waiting for balancing thread to join.");
- Thread.currentThread().interrupt();
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
Review comment:
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: 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] lokeshj1703 merged pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 merged pull request #2578:
URL: https://github.com/apache/ozone/pull/2578
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop interrupts the balancing thread before letting it join.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-909144030
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop interrupts the balancing thread before letting it join.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-909144030
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723880467
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -364,10 +376,15 @@ private IterationResult doIteration() {
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning) {
+ checkIterationMoveResults();
+ return IterationResult.ITERATION_INTERRUPTED;
+ }
Review comment:
Included this in the latest commit. Using the `isBalancerRunning` method that's already there.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r717239874
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -411,7 +436,11 @@ private IterationResult doIteration() {
}
}
- // check move results
+ checkIterationMoveResults();
+ return IterationResult.ITERATION_COMPLETED;
+ }
+
+ private void checkIterationMoveResults() {
Review comment:
The introduction of this method also fixes [HDDS-5760](https://issues.apache.org/jira/browse/HDDS-5760). Now, iteration move results are getting checked every time we exit an iteration early.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r704308026
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -722,8 +722,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(100);
Review comment:
Good point, I agree. We probably don't want to use `thread.stop()`. We can check the `balancerRunning` flag and interrupted status regularly while performing long operations in `doIteration` and `initializeIteration`. This will allow the `currentBalancingThread` to stop gracefully.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723190844
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
Another way could be moving the
`if (!balancerRunning) {
LOG.info("Container Balancer is not running.");
return;
}`
block before the lock is acquired in `stop`.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r720363093
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
+ }
// allow garbage collector to collect balancing thread
currentBalancingThread = null;
Review comment:
Okay, agreed!
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723880467
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -364,10 +376,15 @@ private IterationResult doIteration() {
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning) {
+ checkIterationMoveResults();
+ return IterationResult.ITERATION_INTERRUPTED;
+ }
Review comment:
Included this in the latest commit. Using the `isBalancerRunning` method that's already there.
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r730787766
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -754,17 +773,21 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
-
- // allow garbage collector to collect balancing thread
- currentBalancingThread = null;
- } catch (InterruptedException e) {
- LOG.warn("Interrupted while waiting for balancing thread to join.");
- Thread.currentThread().interrupt();
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
Review comment:
NIT: Move this alongside join call.
--
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] JacksonYao287 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r700865242
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -722,8 +722,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(100);
Review comment:
to make it simple , maybe we can use the deprecated method `thread.stop()`. but this looks not graceful....
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r704308026
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -722,8 +722,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(100);
Review comment:
Good point, I agree. We probably don't want to use `thread.stop()`. We can check the balancerRunning flag and interrupted status regularly while performing long operations in `doIteration` and `initializeIteration`. This will allow the `currentBalancingThread` to stop gracefully.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718503110
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -184,7 +184,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
doIteration();
Review comment:
Checking if balancer is running also inherently means checking for `ITERATION_INTERRUPTED`. So by checking `isBalancerRunning` we can check both: If iteration was interrupted and if balancer was stopped (after intializing the iteration) from CLI.
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723158468
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
It is a Reentrant lock.
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-909155415
Updated the PR and related JIRA to reflect this discussion and the new changes.
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-930170817
Updated according to review!
--
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] JacksonYao287 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718257878
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
Review comment:
NIT: better to use `join()` instead of `join(0)`, they are equal
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -348,17 +361,24 @@ private boolean initializeIteration() {
}
private IterationResult doIteration() {
+ // note that potential and selected targets are updated in the following
+ // loop
List<DatanodeDetails> potentialTargets = getPotentialTargets();
Set<DatanodeDetails> selectedTargets =
new HashSet<>(potentialTargets.size());
moveSelectionToFutureMap = new HashMap<>(unBalancedNodes.size());
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning || Thread.currentThread().isInterrupted()) {
Review comment:
when reaching here, is there a case `balancerRunning` is true and meanwhile `Thread.currentThread().isInterrupted()` is true? we can only check `balancerRunning`, maybe that is enough
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
+ }
// allow garbage collector to collect balancing thread
currentBalancingThread = null;
Review comment:
maybe we could ignore catching InterruptedException here , `finally` is enough.
no matter `stop` is called by balancing thread itself or command line, the lock will be hold, thus when `join`, no `interrupt()` could be called.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
Review comment:
the only `interrupt()` is called in `stop()` , so if we reach here, `isBalancerRunning()` will always return false.
we can remove these code , and this also avoids dead lock
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
}
}
}
- stop();
+ if (!isBalancerRunning()) {
Review comment:
seems we should remove `!` here , or there may be a dead lock
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -184,7 +184,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
doIteration();
Review comment:
NIT: may be we can use the following code here,
```
if (doIteration() = IterationResult.ITERATION_INTERRUPTED) {
return;
}
```
and remove the following
```
// return if balancing has been stopped
if (!isBalancerRunning()) {
return;
}
```
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
Review comment:
the only `interrupt()` is called in `stop()` , so if we reach here, `isBalancerRunning()` will always return false.
maybe we can remove these 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: 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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723152657
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
@lokeshj1703
> Why do we need isBalancerRunning check before calling stop method? It should be ok to call stop directly.
I was thinking of this case:
Suppose `stop()` is called by the CLI. In the stop method, `balancerRunning` would be set to false and `currentBalancingThread` would be interrupted. Then `join()` will wait for `currentBalancingThread` to die while holding the lock. If `currentBalancingThread` now calls `stop()`, it will get disabled since the lock isn't available. This is a possible deadlock.
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723119882
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -185,7 +185,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (isBalancerRunning()) {
+ stop();
Review comment:
Why do we need isBalancerRunning check before calling stop method? It should be ok to call stop directly.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -364,10 +376,15 @@ private IterationResult doIteration() {
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning) {
+ checkIterationMoveResults();
+ return IterationResult.ITERATION_INTERRUPTED;
+ }
Review comment:
I would recommend moving this to a separate function for checking balancer status.
Instead of calling `checkIterationMoveResults` multiple times, maybe throw InterruptedException in the new function.
```
doIteration {
try {
checkBalancerStatus();
...
} catch (InterrupedException e) {
return IterationResult.ITERATION_INTERRUPTED;
} finally {
checkIterationMoveResults();
}
}
```
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -185,7 +185,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (isBalancerRunning()) {
Review comment:
Why would this lead to deadlock? We only have one lock in balancer.
If stop is called multiple times, all subsequent calls should be quick to execute.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -185,7 +185,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (isBalancerRunning()) {
+ stop();
Review comment:
This check is made at multiple places. I think we can avoid this check.
--
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] JacksonYao287 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718435335
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
Review comment:
the only `interrupt()` is called in `stop()` , so if we reach here, `isBalancerRunning()` will always return false.
maybe we can remove these 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: 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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-932021033
@JacksonYao287 Can you please review? Thanks.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718453987
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
}
}
}
- stop();
+ if (!isBalancerRunning()) {
Review comment:
Yes, I meant to check `isBalancerRunning` without the `!` to avoid a deadlock. Thanks for pointing this mistake out!
--
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] JacksonYao287 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718257878
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
Review comment:
NIT: better to use `join()` instead of `join(0)`, they are equal
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -348,17 +361,24 @@ private boolean initializeIteration() {
}
private IterationResult doIteration() {
+ // note that potential and selected targets are updated in the following
+ // loop
List<DatanodeDetails> potentialTargets = getPotentialTargets();
Set<DatanodeDetails> selectedTargets =
new HashSet<>(potentialTargets.size());
moveSelectionToFutureMap = new HashMap<>(unBalancedNodes.size());
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning || Thread.currentThread().isInterrupted()) {
Review comment:
when reaching here, is there a case `balancerRunning` is true and meanwhile `Thread.currentThread().isInterrupted()` is true? we can only check `balancerRunning`, maybe that is enough
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
+ }
// allow garbage collector to collect balancing thread
currentBalancingThread = null;
Review comment:
maybe we could ignore catching InterruptedException here , `finally` is enough.
no matter `stop` is called by balancing thread itself or command line, the lock will be hold, thus when `join`, no `interrupt()` could be called.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
Review comment:
the only `interrupt()` is called in `stop()` , so if we reach here, `isBalancerRunning()` will always return false.
we can remove these code , and this also avoids dead lock
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
}
}
}
- stop();
+ if (!isBalancerRunning()) {
Review comment:
seems we should remove `!` here , or there may be a dead lock
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -184,7 +184,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
doIteration();
Review comment:
NIT: may be we can use the following code here,
```
if (doIteration() = IterationResult.ITERATION_INTERRUPTED) {
return;
}
```
and remove the following
```
// return if balancing has been stopped
if (!isBalancerRunning()) {
return;
}
```
--
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] lokeshj1703 commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-939874743
CompletableFuture would still need a thread/executor to execute on. Otherwise it uses a shared pool which is not ideal.
I would suggest to cover that in a separate PR.
Further we will also need to check if future can be interrupted.
--
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] lokeshj1703 commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-946388831
@JacksonYao287 do you have any other comments?
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop interrupts the balancing thread before letting it join.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-906934463
@lokeshj1703 @JacksonYao287 please review this, thanks.
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-930170817
Updated according to review!
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r721093898
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -185,7 +185,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (isBalancerRunning()) {
Review comment:
I was thinking about this too. Can we do anything to protect against this situation?
--
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] JacksonYao287 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r720606547
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -185,7 +185,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (isBalancerRunning()) {
Review comment:
i think there may be a very corner case.
```
if (isBalancerRunning()) {
// at this time , `stop` may be called from command line and the lock is hold.
// so, the `stop` here will stuck in a deadlock
stop();
}
```
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723161794
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
In that case, move join call outside the lock.
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop interrupts the balancing thread before letting it join.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r697292681
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -722,8 +722,8 @@ public void stop() {
return;
}
balancerRunning = false;
+ currentBalancingThread.join(500);
Review comment:
join is a waiting call. This will return when the thread stops or after timeout of 500ms.
The existing logic seems good. Interrupt is required to get the thread out of wait state so that close can finish faster.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718451563
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -348,17 +361,24 @@ private boolean initializeIteration() {
}
private IterationResult doIteration() {
+ // note that potential and selected targets are updated in the following
+ // loop
List<DatanodeDetails> potentialTargets = getPotentialTargets();
Set<DatanodeDetails> selectedTargets =
new HashSet<>(potentialTargets.size());
moveSelectionToFutureMap = new HashMap<>(unBalancedNodes.size());
// match each overUtilized node with a target
for (DatanodeUsageInfo datanodeUsageInfo : overUtilizedNodes) {
+ if (!balancerRunning || Thread.currentThread().isInterrupted()) {
Review comment:
Yes, I think checking only `balancerRunning` is enough. I'll change this.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -203,13 +205,17 @@ private void balance() {
} catch (InterruptedException e) {
LOG.info("Container Balancer was interrupted while waiting for" +
" next iteration.");
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
}
}
}
- stop();
+ if (!isBalancerRunning()) {
Review comment:
Yes, I meant to check `isBalancerRunning` without the `!` to avoid a deadlock. Thanks for pointing this mistake out!
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
+ }
// allow garbage collector to collect balancing thread
currentBalancingThread = null;
Review comment:
Could `interrupt` be called on balancer from a test method?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -184,7 +184,9 @@ private void balance() {
for (int i = 0; i < idleIteration && balancerRunning; i++) {
// stop balancing if iteration is not initialized
if (!initializeIteration()) {
- stop();
+ if (!isBalancerRunning()) {
+ stop();
+ }
return;
}
doIteration();
Review comment:
Checking if balancer is running also inherently means checking for `ITERATION_INTERRUPTED`. So by checking `isBalancerRunning` we can check both: If iteration was interrupted and if balancer was stopped (after intializing the iteration) from CLI.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r717239874
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -411,7 +436,11 @@ private IterationResult doIteration() {
}
}
- // check move results
+ checkIterationMoveResults();
+ return IterationResult.ITERATION_COMPLETED;
+ }
+
+ private void checkIterationMoveResults() {
Review comment:
The introduction of this method also fixes [HDDS-5760](https://issues.apache.org/jira/browse/HDDS-5760). Now, iteration move results are getting checked every time we exit an iteration early.
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-945468442
@JacksonYao287 @lokeshj1703 thanks for the reviews. I've updated the PR, please take a look.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723192328
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
> You will need to make sure that start does not succeed until join has completed for the older thread.
This seems a bit difficult at first glance. I'll have to think about it 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] JacksonYao287 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r720328164
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
+ }
// allow garbage collector to collect balancing thread
currentBalancingThread = null;
Review comment:
i think in test method , `containerbalancer#stop` may be called , but `interrupt` will never be called directly because `currentBalancingThread` is private.
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop interrupts the balancing thread before letting it join.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-909144030
Added a check to prevent the current thread from interrupting itself. Also reverted the order of interrupt and join calls to the existing logic according to comment from @lokeshj1703
--
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] siddhantsangwan commented on pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#issuecomment-928067819
@JacksonYao287 I've updated this PR. Regularly checking for `balancerRunning` and interrupted statuses in long running operations.
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r718463542
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -718,8 +747,10 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join(0);
+ }
// allow garbage collector to collect balancing thread
currentBalancingThread = null;
Review comment:
Could `interrupt` be called on balancer from a test method?
--
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] siddhantsangwan commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r723188402
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
How about instead of calling lock(), we use tryLock()? If tryLock() returns false then the lock is already held in either the start or the stop method. In both the cases, we can just return from the stop method.
--
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] lokeshj1703 commented on a change in pull request #2578: HDDS-5671. ContainerBalancer#stop should prevent the current balancing thread from interrupting itself.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2578:
URL: https://github.com/apache/ozone/pull/2578#discussion_r724859175
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -743,14 +768,14 @@ public void stop() {
return;
}
balancerRunning = false;
- currentBalancingThread.interrupt();
- currentBalancingThread.join(1000);
+ if (Thread.currentThread().getId() != currentBalancingThread.getId()) {
+ currentBalancingThread.interrupt();
+ currentBalancingThread.join();
+ }
Review comment:
I would suggest to move join outside the lock. join also takes a lock on Thread. I would recommend to avoid using two locks.
--
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