-
Notifications
You must be signed in to change notification settings - Fork 499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-11367. Improve ozone balancing status command output #7139
base: master
Are you sure you want to change the base?
Conversation
f6f34c0
to
9f928d1
Compare
9f928d1
to
00ad471
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juncevich can you please elaborate on what these points mean?
job task time consumption
iteration time consumption
It will help reviewers understand what you're proposing.
Hi, @siddhantsangwan. Sure, i can explain. Balancing job constists from some iterations. Job task time - the whole balancing time, iteration time - particular iteration time. Job time = iteration time + iteration time ... |
...ds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java
Outdated
Show resolved
Hide resolved
Hi siddhantsangwan. Can you review my PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juncevich thanks for working on this. Other than my comments below, we definitely need more testing here. I'd like to at least see:
- Unit tests for balancer that verify the iteration duration and other properties are correct.
- Tests for the balancer status command that verify the code is correctly picking up the mocked values (mocked response from SCM).
We can also add verifications to the robot tests.
...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...ds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java
Outdated
Show resolved
Hide resolved
"Scheduled to move containers", containerMovesScheduled, | ||
"Already moved containers", containerMovesCompleted, | ||
"Failed to move containers", containerMovesFailed, | ||
"Failed to move containers by timeout", containerMovesTimeout, | ||
"Entered data to nodes", enteringDataNodeList, | ||
"Exited data from nodes", leavingDataNodeList); | ||
} | ||
|
||
private String getPrettyIterationStatusInfo(long duration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably existing utility methods to do this as well. See if you can find any? It's better to use them since they will be well tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find utility methods. I will extract this method to separate util class and write test for it.
...r-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerStatusInfo.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Show resolved
Hide resolved
...ds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java
Show resolved
Hide resolved
"%-50s %n%s" + | ||
"%-50s %n%s", | ||
"Key", "Value", | ||
"Iteration number", iterationNumber, | ||
"Iteration number", iterationNumber == 0 ? "-" : iterationNumber, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what reason are we inserting string constants using String.format? It doesn't feel right quite to be honest. Can we refactor this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why it's bad and how I should refactor it.
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/util/DurationUtil.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/util/DurationUtil.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerStatusInfo.java
Outdated
Show resolved
Hide resolved
...r-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...r-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java
Outdated
Show resolved
Hide resolved
@juncevich can you please check test timeouts / failures?
https://github.com/juncevich/ozone/actions/runs/11380961580/artifacts/2067990610 |
Thanks for the comment. Tests fixed. |
@siddhantsangwan can you please take another look? |
# Conflicts: # hadoop-ozone/dist/src/main/smoketest/balancer/testBalancer.robot
@juncevich Is this ready for another round of reviews? |
Yes, I try to fix the review notices ASAP because they are blocking another fix. I didn’t bring it up because it’s a different task |
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
Outdated
Show resolved
Hide resolved
...r-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
Show resolved
Hide resolved
@@ -118,7 +121,7 @@ public class ContainerBalancerTask implements Runnable { | |||
private int nextIterationIndex; | |||
private boolean delayStart; | |||
private List<ContainerBalancerTaskIterationStatusInfo> iterationsStatistic; | |||
|
|||
private OffsetDateTime currentIterationStarted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst it maybe updated in one thread read access can be done from multiple other threads. This variable should be Atomic.
...r-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestContainerBalancerSubCommand.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestContainerBalancerSubCommand.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestContainerBalancerSubCommand.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestContainerBalancerSubCommand.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestContainerBalancerSubCommand.java
Show resolved
Hide resolved
@ivanzlenko No need to use Atomic for OffsetDateTime, because it thread-safe by design. |
private final Map<UUID, Long> sizeEnteringNodesGB; | ||
private final Map<UUID, Long> sizeLeavingNodesGB; | ||
private final Map<UUID, Long> sizeEnteringNodes; | ||
private final Map<UUID, Long> sizeLeavingNodes; | ||
|
||
@SuppressWarnings("checkstyle:ParameterNumber") | ||
public ContainerBalancerTaskIterationStatusInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is NOT okay to follow bad practices. Please fix it.
What changes were proposed in this pull request?
Minor improvements to command output:
What is the link to the Apache JIRA
HDDS-11367
How was this patch tested?
Tested by robot test