HDDS-4926. Support start/stop for container balancer via command line #2278

JacksonYao287 · 2021-05-24T06:08:16Z

What changes were proposed in this pull request?

Support start/stop for container balancer via command line

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4926

How was this patch tested?

unit and acceptance tests

JacksonYao287 · 2021-05-24T06:08:42Z

@lokeshj1703 PTAL

GlenGeng-awx · 2021-05-24T08:12:09Z

...mmon/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java

+  /**
+   * Start ContainerBalancer.
+   */
+  void startContainerBalancer() throws IOException;


According to our design decision, should we add the parameters like threshold and consecutiveIdleIterations in the start ?

@GlenGeng Thanks for the review! will add these and some usage info.

updated. PTAL

Seems these params are not passed from these RPCs in StorageContainerLocationProtocol ?

yes , i keep it like this , because for now we can not determine how these parameters will be used or should we add more parameters(like bandwidth, which determines the maximum speed at which a block will be moved from one datanode to another ). I just implement the simplest function for now, and i think it will not be hard to complete the full implementation of this after we review the design doc and make the final decision.

public void execute(ScmClient scmClient) throws IOException { scmClient.startContainerBalancer(); LOG.info("Starting ContainerBalancer..."); //remove these in later implementation LOG.info("threshold: {}", threshold); LOG.info("idleiterations: {}", iCount); }

lokeshj1703

@JacksonYao287 Thanks for working on this! The changes look good to me. Can we add UT or acceptance tests for the new API/commands?

...dds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java

siddhantsangwan

The changes look great @JacksonYao287 Can you please address my comments related to configs?

.../apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java

siddhantsangwan · 2021-05-27T07:32:32Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java


    // initialise configs
-    this.config = balancerConfiguration;
+    //this.config = balancerConfiguration;


ContainerBalancerConfiguration can be updated with any CLI params and then used as a single source of configurations wherever needed.

@siddhantsangwan Thanks for the review! we can just specify some important parameters for now I think and then keep on adding params as required . we can add more params as we move forward with the implementation.

Since these parameters are required in multiple classes, we can set these fields in the ContainerBalancerConfiguration class and then pass this object to wherever parameters are required. This way, all parameters will be present in one place instead of being get/set in different places. It could be better to maintain all the parameters in a single class. What do you think?

ChenSammi · 2021-06-02T02:57:23Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java

Can we use K-V map to hold all parameters here, to avoid API change in future in case introducting new parameters

ChenSammi · 2021-06-02T02:59:29Z

...mmon/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java

same as above.

ChenSammi · 2021-06-02T03:00:48Z

...pache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java

Unnecessary blank line can be removed.

thanks , will remove it

Can we remove these extra head and tail black line?

ChenSammi · 2021-06-02T03:10:11Z

hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto

Can we using KeyValue here if there will be more fields in future.

Do all these required fileds have default values? If so, they are optional, insteal of required.

Thanks @ChenSammi for the reivew. there will not be default value, see this discuss

I think @ChenSammi is pointing out that since all these fields do not have default values they should be optional.

JacksonYao287 · 2021-06-03T08:02:48Z

@adoroszlai Hey，can you help having a look at this CI problem TestReportPublisher.testCRLStatusReportPublisher? it seems nothing to do with my PR, i do not modify any CRL related code.

adoroszlai · 2021-06-03T08:11:37Z

Hey，can you help having a look at this CI problem TestReportPublisher.testCRLStatusReportPublisher? it seems nothing to do with my PR, i do not modify any CRL related code.

Yes, it's unrelated. #2302 fixes it. Please wait for that to be merged first.

JacksonYao287 · 2021-06-03T08:14:02Z

thanks @adoroszlai for looking at this!

Yes, it's unrelated. #2302 fixes it. Please wait for that to be merged first.

will wait for that!

merge the lastest master branch to get a test bug fix : see HDDS-5295

lokeshj1703

@JacksonYao287 Thanks for updating! The changes look great. I have few comments inline.

lokeshj1703 · 2021-06-04T15:12:44Z

hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto

I think @ChenSammi is pointing out that since all these fields do not have default values they should be optional.

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

...r-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java

...dds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java

lokeshj1703

@JacksonYao287 Thanks for updating the PR! The changes look good to me. +1.

lokeshj1703 · 2021-06-21T10:17:06Z

@GlenGeng @ChenSammi @siddhantsangwan Do you have any other comments?

ChenSammi · 2021-06-23T02:15:52Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java

+   * Start ContainerBalancer.
+   */
+  boolean startContainerBalancer(Optional<Double> threshold,
+         Optional<Integer> idleiterations,


We use 4 black space indent when a line wraps.

ChenSammi · 2021-06-23T02:16:12Z

...mmon/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java

+  boolean startContainerBalancer(Optional<Double> threshold,
+           Optional<Integer> idleiterations,
+           Optional<Integer> maxDatanodesToBalance,
+           Optional<Long> maxSizeToMoveInGB) throws IOException;


Same as above.

ChenSammi · 2021-06-23T02:19:15Z

...pache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java

+  @Override
+  public boolean getContainerBalancerStatus() throws IOException {
+
+    ContainerBalancerStatusRequestProto request =


Same as above.

seems i have already used 4 black space indent here

ChenSammi · 2021-06-23T02:20:07Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+    lock.lock();
+    try {
+      if (!balancerRunning.compareAndSet(false, true)) {
+        LOG.error("Container Balancer is already running.");


error -> info

ChenSammi · 2021-06-23T02:21:11Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+        return false;
+      }
+
+      ozoneConfiguration = new OzoneConfiguration();


ozoneConfiguration seems not be used later.

JacksonYao287 · 2021-06-23T02:58:00Z

thanks @ChenSammi for the review, i have fix the comment.PTAL!

ChenSammi · 2021-06-23T05:29:52Z

.../main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java

          "by Container Balancer.")
  private long maxSizeToMove = 10 * OzoneConsts.GB;

+  @Config(key = "idle.iterations", type = ConfigType.INT,


Based on current implementation, I would suggest changing the "idle.iterations" to "max.iterations". As Lokesh said, we can do it in a seperate PR later to revisit all these configurations and namings.

yea, we could revisit it later

ChenSammi · 2021-06-23T05:31:55Z

...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java

+                  Optional<Integer> maxDatanodesToBalance,
+                  Optional<Long> maxSizeToMoveInGB) throws IOException{
+    getScm().checkAdminAccess(getRemoteUser());
+    AUDIT.logWriteSuccess(buildAuditMessageForSuccess(


I think we should write the audit log after the balancer is started at the end.

yea, that make sense , i will do it

ChenSammi · 2021-06-23T05:47:30Z

Thanks @JacksonYao287 for working on this. The path LGTM, only several small issues.

siddhantsangwan

I have a few minor comments, please take a look.

siddhantsangwan · 2021-06-23T05:49:22Z

...pache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java

+    if (idleiterations.isPresent()) {
+      int idi = idleiterations.get();
+      Preconditions.checkState(idi > 0 || idi == -1,
+          "maxDatanodesToBalance must be positive or" +


Did you mean:

Suggested change

"maxDatanodesToBalance must be positive or" +

"idleIterations must be positive or" +

siddhantsangwan · 2021-06-23T06:08:39Z

...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java

+    if (idleiterations.isPresent()) {
+      int idi = idleiterations.get();
+      Preconditions.checkState(idi > 0 || idi == -1,
+          "maxDatanodesToBalance must be positive or" +


Did you mean:

Suggested change

"maxDatanodesToBalance must be positive or" +

"idleIterations must be positive or" +

siddhantsangwan · 2021-06-23T06:21:07Z

.../main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java

+   * @param count a idle iteration count larger than 0
+   */
+  public void setIdleIteration(int count) {
+    if (count < 1) {


Aren't we also allowing idleIterations to be -1? In that case this condition needs to be changed.

siddhantsangwan · 2021-06-23T06:24:34Z

...r-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java

  public void containerBalancerShouldStopWhenMaxDatanodesToBalanceIsReached() {
    balancerConfiguration.setMaxDatanodesToBalance(2);
-    balancerConfiguration.setThreshold(0);
+    balancerConfiguration.setThreshold(1);


Why is threshold set to 1 here? A threshold of 1 means balancing is not required, which is incorrect for this test since we are checking the maxDatanodesToBalance configuration. Moreover threshold belongs to [0, 1).

JacksonYao287 · 2021-06-23T07:02:58Z

Thank @siddhantsangwan for the review, i have fixed the comment, PTAL!

ChenSammi · 2021-06-23T11:34:30Z

The last patch LGTM, +1. Thanks @JacksonYao287 for the contribution and @lokeshj1703 @siddhantsangwan for code review.

* master: (28 commits) HDDS-5332. Add a new column family and a service provider in Recon DB for Namespace Summaries (apache#2366) HDDS-5405. Refactor pom files for HadoopRpc and Grpc/Ratis compilation properties. (apache#2386) HDDS-5406. add proto version to all the proto files. (apache#2385) HDDS-5398. Avoid object creation in ReplicationManger debug log statements (apache#2379) HDDS-5396. Fix negligence issue conditional expressions in MockCRLStore.java (apache#2380) HDDS-5395. Avoid unnecessary numKeyOps.incr() call in OMMetrics (apache#2374) HDDS-5389. Include ozoneserviceid in fs.defaultFS when configuring o3fs (apache#2370) HDDS-5383. Eliminate expensive string creation in debug log messages (apache#2372) HDDS-5380. Get more accurate space info for DedicatedDiskSpaceUsage. (apache#2365) HDDS-5341. Container report processing is single threaded (apache#2338) HDDS-5387. ProfileServlet to move the default output location to an ozone specific directory (apache#2368) HDDS-5289. Update container's deleteTransactionId on creation of the transaction in SCM. (apache#2361) HDDS-5369. Cleanup unused configuration related to SCM HA (apache#2359) HDDS-5381. SCM terminated with exit status 1: null. (apache#2362) HDDS-5353. Avoid unnecessary executeBatch call in insertAudits (apache#2342) HDDS-5350 : Add allocate block support in MockOmTransport (apache#2341). Contributed by Uma Maheswara Rao G. HDDS-4926. Support start/stop for container balancer via command line (apache#2278) HDDS-5269. Datandoe with low ratis log volume space should not be considered for new pipeline allocation. (apache#2344) HDDS-5367. Update modification time when updating quota/storageType/versioning (apache#2355) HDDS-5352. java.lang.ClassNotFoundException: org/eclipse/jetty/alpn/ALPN (apache#2347) ...

HDDS-4926. Support start/stop for container balancer via command line

4ec7f55

GlenGeng-awx reviewed May 24, 2021

View reviewed changes

Jackson Yao added 2 commits May 24, 2021 17:07

add command line parameters

f7cea16

fix for checkstyle

8a26e33

lokeshj1703 mentioned this pull request May 26, 2021

HDDS-4927. Determine over and under utilized datanodes in Container Balancer. #2230

Closed

lokeshj1703 reviewed May 27, 2021

View reviewed changes

...dds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStartSubcommand.java Outdated Show resolved Hide resolved

siddhantsangwan reviewed May 27, 2021

View reviewed changes

Jackson Yao added 5 commits May 27, 2021 19:24

remove default value

5efb584

Merge remote-tracking branch 'origin/master' into bak

a96c1ba

update proto to add new parameters

fff764b

add unit test for container balancer subcommand

4526b6a

add unit and acceptance test

558c0fc

JacksonYao287 force-pushed the HDDS-4926 branch from 02e8fb7 to 558c0fc Compare June 1, 2021 12:48

ChenSammi reviewed Jun 2, 2021

View reviewed changes

...mmon/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java Outdated

Copy link

Contributor

ChenSammi Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

ChenSammi reviewed Jun 2, 2021

View reviewed changes

fix unit test

45e79e4

JacksonYao287 force-pushed the HDDS-4926 branch from 9bbb39b to 45e79e4 Compare June 3, 2021 06:50

Jackson Yao added 2 commits June 3, 2021 15:18

triger CI for TestReportPublisher.testCRLStatusReportPublisher

d913cf4

Merge remote-tracking branch 'origin/master' into HDDS-4926

2879fb8

Merge remote-tracking branch 'origin/master' into HDDS-4926

029cf09

merge the lastest master branch to get a test bug fix : see HDDS-5295

JacksonYao287 requested a review from lokeshj1703 June 3, 2021 12:38

lokeshj1703 reviewed Jun 4, 2021

View reviewed changes

Jackson Yao added 2 commits June 7, 2021 14:05

update

c4f2fc4

make startContainerBalancer parameters optional

3cd3fae

JacksonYao287 force-pushed the HDDS-4926 branch from fd81b7c to 6a04ed2 Compare June 21, 2021 07:46

JacksonYao287 requested a review from lokeshj1703 June 21, 2021 07:54

lokeshj1703 reviewed Jun 21, 2021

View reviewed changes

JacksonYao287 requested a review from ChenSammi June 22, 2021 15:41

Merge remote-tracking branch 'origin/master' into HDDS-4926

00e34f4

ChenSammi reviewed Jun 23, 2021

View reviewed changes

update

50029a7

JacksonYao287 requested review from ChenSammi and lokeshj1703 June 23, 2021 04:50

ChenSammi reviewed Jun 23, 2021

View reviewed changes

update

0199f26

siddhantsangwan reviewed Jun 23, 2021

View reviewed changes

JacksonYao287 force-pushed the HDDS-4926 branch from 5426344 to 44aa8b1 Compare June 23, 2021 07:05

Jackson Yao added 4 commits June 23, 2021 15:05

update

44aa8b1

triger CI

e7abf32

triger CI

e99c865

triger CI

b69c5c3

ChenSammi merged commit dc02775 into apache:master Jun 23, 2021

JacksonYao287 deleted the HDDS-4926 branch July 20, 2021 11:45

	"maxDatanodesToBalance must be positive or" +
	"idleIterations must be positive or" +

HDDS-4926. Support start/stop for container balancer via command line #2278

HDDS-4926. Support start/stop for container balancer via command line #2278

Uh oh!

Conversation

JacksonYao287 commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

JacksonYao287 commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lokeshj1703 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siddhantsangwan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 commented Jun 3, 2021

Uh oh!

adoroszlai commented Jun 3, 2021

Uh oh!

JacksonYao287 commented Jun 3, 2021

Uh oh!

lokeshj1703 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lokeshj1703 left a comment

Choose a reason for hiding this comment

Uh oh!

lokeshj1703 commented Jun 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 commented May 24, 2021 •

edited

Loading

JacksonYao287 May 26, 2021 •

edited

Loading

ChenSammi Jun 23, 2021 •

edited

Loading