HDDS-7155. [DiskBalancer] Create interface between SCM and DN #3701

symious · 2022-08-20T15:24:27Z

What changes were proposed in this pull request?

This ticket is for creating the interface between SCM and DN. The new interfaces are as follows:

SCM -> DN
- SCM send DiskBalancerCommand to datanodes to control/update diskBalancer
DN -> SCM
- DN report DiskBalancerReport to SCM via heartbeat

What is the link to the Apache JIRA

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

How was this patch tested?

unit test.

symious · 2022-08-20T15:26:19Z

hadoop-hdds/interface-client/src/main/proto/hdds.proto

 message DiskBalancerConfigurationProto {
-    optional double threshold = 1;
-    optional double diskBandwidth = 2;
+    optional string threshold = 1;


@ChenSammi Currently ConfigurationReflectionUtil can not detect the type of "Double", so I revert the type back to string, please have a look.

I see. Do you think it's difficult to add a double support in ConfigurationReflectionUtil?

Added "Double" to ConfigType in another PR, please have a look.
https://issues.apache.org/jira/browse/HDDS-7162

symious · 2022-08-20T15:26:33Z

@ChenSammi @ferhui @lokeshj1703 @siddhantsangwan @sodonnel @neils-dev @JacksonYao287 Could you help to review this PR?

myskov · 2022-08-21T14:37:46Z

...he/hadoop/ozone/container/common/statemachine/commandhandler/DiskBalancerCommandHandler.java

+//    DiskBalancerCommand diskBalancerCommand = (DiskBalancerCommand) command;
+//    final boolean shouldRun = diskBalancerCommand.isShouldRun();
+//    final DiskBalancerConfiguration configuration =
+//        diskBalancerCommand.getDiskBalancerConfiguration();


this code should be either implemented or removed. The code that is commented out, should not go to production code

Thanks for the review, I will comment it.

myskov · 2022-08-21T16:39:48Z

...c/main/java/org/apache/hadoop/ozone/container/common/report/DiskBalancerReportPublisher.java

+
+
+/**
+ * Publishes Pipeline which will be sent to SCM as part of heartbeat.


seems to be a copy-paste from PipelineReportPublisher.class

ferhui · 2022-08-22T07:52:47Z

Hi @symious , Sammi @ChenSammi gave a comment 'Please use uint64 instead. diskBandwidth is a number without fraction. We can change it in next PR.' in the last PR. Could you change it in this PR?

hadoop-hdds/interface-client/src/main/proto/hdds.proto

ChenSammi · 2022-08-23T05:28:12Z

...-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/DiskBalancerConfiguration.java

      defaultValue = "10", tags = {ConfigTag.DISKBALANCER},
      description = "The max balance speed.")
-  private double diskBandwidth = 10;
+  private long diskBandwidth = 10;


Please check the type of all key, use the proper type instead of ConfigType.AUTO if possible.

ChenSammi · 2022-08-23T05:38:46Z

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

+
+      if (heartbeat.hasDiskBalancerReport()) {
+        LOG.debug("Dispatching DiskBalancer Report.");
+        eventPublisher.fireEvent(


Copy-paste result?

Yep, let me fix it.

ChenSammi · 2022-08-23T05:43:30Z

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

+      diskBalancerManager.processDiskBalancerReport(
+          diskBalancerReportProto, dn);
+    } catch (Exception e) {
+      LOGGER.error("Could not process diskBalancer report={} from dn={}.",


Could not process -> Failed to process

ChenSammi · 2022-08-23T06:09:15Z

...c/main/java/org/apache/hadoop/ozone/container/common/report/DiskBalancerReportPublisher.java

+      Preconditions.checkState(
+          heartbeatFrequency <= diskBalancerReportInterval,
+              HDDS_DISK_BALANCER_REPORT_INTERVAL +
+              " cannot be configured lower than heartbeat frequency.");


Please add the heart frequency value in the message to facilitate problem investigation.

ChenSammi · 2022-08-23T06:24:59Z

...he/hadoop/ozone/container/common/statemachine/commandhandler/DiskBalancerCommandHandler.java

+    // Todo: add implementation to stop DiskBalancer
+  }
+
+  private void updateDiskBalancerConfiguration(DiskBalancerConfiguration


updateDiskBalancerConfiguration -> updateDiskBalancer

ChenSammi · 2022-08-23T06:27:18Z

hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto


+message DiskBalancerReportProto {
+  required bool isRunning = 1;
+  optional int64 balancedBytes = 2;


int64 -> uint64

ChenSammi · 2022-08-23T06:36:01Z

Hey @symious , I finished one round of review and left some comments.

symious · 2022-08-23T06:38:15Z

@ChenSammi Thank you for the detailed review. I will update the PR later.

For the "Double" type issue, I will create another PR in master, hope you can have a look and we can rebase and use it in the current branch. Thank you again.

symious · 2022-08-26T07:10:39Z

@ferhui Thanks for updating the patch.
@ChenSammi Conflicts after rebased has been solved, please have a look.

ferhui

+1

lokeshj1703

@symious Thanks for updating the PR! I have few comments inline.

lokeshj1703 · 2022-09-01T06:18:23Z

...ervice/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java

@@ -112,11 +112,11 @@ public class StateContext {
  private final ConfigurationSource conf;
  private final Set<InetSocketAddress> endpoints;
  // Only the latest full report of each type is kept
-  private final AtomicReference<GeneratedMessage> containerReports;


Why do we need this change?

It is to resolve the conflicts with the change of HDDS-7151.

lokeshj1703 · 2022-09-01T10:25:03Z

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

-    return statusList;
+    // Filter Running Status by default
+    HddsProtos.DiskBalancerRunningStatus filterStatus =
+        status.orElse(HddsProtos.DiskBalancerRunningStatus.RUNNING);


Lets not filter by default.

lokeshj1703 · 2022-09-01T10:26:17Z

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

+          if (runningStatus != HddsProtos.DiskBalancerRunningStatus.UNKNOWN) {
+            builder.setDiskBalancerConf(statusMap.get(dn)
+                .getDiskBalancerConfiguration().toProtobufBuilder());
+          }


Do we need to report conf for STOPPED status dns?

If user specified the STOPEED status dn, I think we should return the information.

But for STOPPED balancer, there should not exist any config right?

I think they should have a local copy of configuration. So that they can use the latest configuration when we start them.

lokeshj1703 · 2022-09-01T10:31:56Z

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

-      return hosts.contains(datanodeDetails);
+    boolean shouldReturn = true;
+    // If dn not in specified hosts, do not return
+    if (hosts != null && !hosts.isEmpty() && !hosts.contains(datanodeDetails)) {


!hosts.isEmpty() We do not need this check.

lokeshj1703 · 2022-09-01T10:32:59Z

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

+
+    List<DatanodeDetails> finalFilterDns = filterDns;
+
+    return nodeManager.getAllNodes().stream()


We are still looping through all nodes here. In case hosts is not empty we should only loop through nodes provided by the hosts input parameter.

Updated the for loop logic.

ferhui · 2022-09-03T16:12:51Z

@ChenSammi @myskov @lokeshj1703 Thanks for your careful reviews! Do you have any other comments?

ferhui · 2022-09-06T03:28:44Z

Seems no further comments. Will merge it tmr so that @symious can continue.

ChenSammi · 2022-09-06T05:47:07Z

The patch LGTM, +1.

ferhui · 2022-09-07T04:22:38Z

@symious Thanks for your contribution. @myskov @ChenSammi @lokeshj1703 Thanks for your careful reviews! Merged.

symious · 2022-09-07T06:50:37Z

@ChenSammi @ferhui @lokeshj1703 @myskov Thank you for the review.

…#3701)

symious and others added 2 commits August 16, 2022 10:40

HDDS-7106. [DiskBalancer] Client-SCM interface (apache#3663)

8e31b09

HDDS-7155. Create interface between SCM and DN

d239e2f

symious commented Aug 20, 2022

View reviewed changes

symious changed the title ~~HDDS-7155. Create interface between SCM and DN~~ HDDS-7155. [DiskBalancer] Create interface between SCM and DN Aug 21, 2022

symious added 4 commits August 21, 2022 08:52

HDDS-7155. Fix unit test and comment unused variable

4a07bd2

HDDS-7155. add config in ozone-site.xml

cb8844f

trigger new CI check

892e19b

trigger new CI check

be77b11

myskov reviewed Aug 21, 2022

View reviewed changes

symious added 2 commits August 21, 2022 22:44

HDDS-7155. Comment some code

120f350

HDDS-7155. Add variable of totalTime

57b3c90

myskov reviewed Aug 21, 2022

View reviewed changes

HDDS-7155. Fix copied comments

385be29

symious commented Aug 22, 2022

View reviewed changes

hadoop-hdds/interface-client/src/main/proto/hdds.proto Outdated Show resolved Hide resolved

ChenSammi reviewed Aug 23, 2022

View reviewed changes

HDDS-7155. Fix copy-paste result

538752e

ChenSammi reviewed Aug 23, 2022

View reviewed changes

HDDS-7155. Fix as comments

da2c777

ferhui force-pushed the HDDS-5713 branch from 8e31b09 to 09a0b8e Compare August 25, 2022 03:55

symious and others added 2 commits August 26, 2022 14:16

Merge branch 'HDDS-5713' into HDDS-7155

9129fb3

HDDS-7155. Fix conflicts

d9ae389

symious added 3 commits August 30, 2022 10:49

trigger new CI check

60498d6

HDDS-7155. Add Op proto

f07c6df

HDDS-7155. Update return types for SCMClient

e1b7bf6

ferhui approved these changes Sep 1, 2022

View reviewed changes

HDDS-7155. Update infoType to opType

f47f70b

lokeshj1703 reviewed Sep 1, 2022

View reviewed changes

symious added 6 commits September 1, 2022 19:54

HDDS-7155. Update for loop

b2dc15c

HDDS-7155. Fix checkStyle

8f8d445

trigger new CI check

994dd8b

HDDS-7155. Use Double compare

a405f96

trigger new CI check

b1e8df3

trigger new CI check

165c814

ferhui merged commit b239018 into apache:HDDS-5713 Sep 7, 2022

symious added a commit that referenced this pull request Feb 12, 2023

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (#3701)

c4a4cc0

symious added a commit to symious/ozone that referenced this pull request Mar 16, 2023

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

248af79

…#3701)

symious added a commit to symious/ozone that referenced this pull request Jan 29, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

f8ebf47

…#3701)

symious added a commit that referenced this pull request Jan 29, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (#3701)

c976a50

sodonnel pushed a commit to sodonnel/hadoop-ozone that referenced this pull request Feb 1, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

889f86f

…#3701)

asfgit pushed a commit that referenced this pull request Feb 1, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (#3701)

b4fa351

symious added a commit that referenced this pull request Mar 17, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (#3701)

9014a79

sadanand48 pushed a commit to sadanand48/hadoop-ozone that referenced this pull request Apr 17, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

501d170

…#3701)

sadanand48 pushed a commit to sadanand48/hadoop-ozone that referenced this pull request Apr 17, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

33c11cb

…#3701)

sadanand48 pushed a commit that referenced this pull request Apr 30, 2024

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (#3701)

96e0342

ChenSammi pushed a commit to ChenSammi/ozone that referenced this pull request Feb 19, 2025

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

af73c7b

…#3701)

ChenSammi pushed a commit to ChenSammi/ozone that referenced this pull request Feb 20, 2025

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

1665d86

…#3701)



		/**
		* Publishes Pipeline which will be sent to SCM as part of heartbeat.


		List<DatanodeDetails> finalFilterDns = filterDns;

		return nodeManager.getAllNodes().stream()

HDDS-7155. [DiskBalancer] Create interface between SCM and DN #3701

HDDS-7155. [DiskBalancer] Create interface between SCM and DN #3701

Uh oh!

Conversation

symious commented Aug 20, 2022

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

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!

symious commented Aug 20, 2022

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!

ferhui commented Aug 22, 2022

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!

ChenSammi Aug 23, 2022 • 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!

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!

ChenSammi commented Aug 23, 2022

Uh oh!

symious commented Aug 23, 2022

Uh oh!

symious commented Aug 26, 2022

Uh oh!

ferhui left a comment

Choose a reason for hiding this comment

Uh oh!

lokeshj1703 left a comment

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!

ferhui commented Sep 3, 2022

Uh oh!

ferhui commented Sep 6, 2022

Uh oh!

ChenSammi commented Sep 6, 2022

Uh oh!

ChenSammi Aug 23, 2022 •

edited

Loading