You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "shuan1026 (via GitHub)" <gi...@apache.org> on 2026/10/13 02:21:04 UTC

[PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

shuan1026 opened a new pull request, #11503:
URL: https://github.com/apache/ozone/pull/11503

   ## What changes were proposed in this pull request?
   
   **Bug:** On first DiskBalancer init (`diskBalancer.info` absent), only `should.run.default` was taken from injected config; threshold, bandwidth, parallel thread, stop-after-disk-even, and container states were seeded via `new DiskBalancerConfiguration()` and persisted as Java defaults. Later restarts keep the wrong file; XML changes alone do not fix it.
   
   **Impact:** Operator `ozone-site.xml` tuning is ignored on new datanodes. Balancing may use wrong I/O limits / stop condition / movable states; `diskbalancer status` reports the persisted defaults with no warning.
   
   **Fix:** Seed initial `DiskBalancerInfo` from `conf.getObject(DiskBalancerConfiguration.class)`. Existing-file load path unchanged.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-16965
   
   ## How was this patch tested?
   
   - Added `TestDiskBalancerService#testInitialDiskBalancerInfoUsesConfiguredValues`: empty info dir + non-default config via `setFromObject`; asserts in-memory info and written YAML match configured values.
   - ci: https://github.com/shuan1026/ozone/actions/runs/38335830063


-- 
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

Re: [PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #11503:
URL: https://github.com/apache/ozone/pull/11503#issuecomment-6252280411

   Thanks @shuan1026 for the patch, @Gargi-jais11, @slfan1989 for the 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

Re: [PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #11503:
URL: https://github.com/apache/ozone/pull/11503#discussion_r4267746975


##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerConfiguration.java:
##########
@@ -169,6 +181,28 @@ public DiskBalancerConfiguration(Double threshold,
   public DiskBalancerConfiguration() {
   }
 
+  @PostConstruct
+  public void validate() {
+    if (threshold <= 0d || threshold >= 100d) {

Review Comment:
   `NaN` bypasses this validation because both comparisons evaluate to `false`. 
   
   Since the configuration parser uses `Double.parseDouble()`, an XML value of `NaN` reaches this method and is subsequently persisted to `diskBalancer.info`.
   
   Once balancing runs, comparisons against the resulting `NaN` threshold are always false. For example, `newUtilization >= upperThreshold` cannot reject a move, which may cause containers to be selected without a valid utilization limit.
   
   Could we reject non-finite values here and in `setThreshold()`, for example:
   
   ```
   if (!Double.isFinite(threshold) || threshold <= 0d || threshold >= 100d)
   ```
   
   Please also add a test using a raw `NaN` configuration value.



-- 
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

Re: [PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #11503:
URL: https://github.com/apache/ozone/pull/11503


-- 
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

Re: [PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

Posted by "shuan1026 (via GitHub)" <gi...@apache.org>.
shuan1026 commented on code in PR #11503:
URL: https://github.com/apache/ozone/pull/11503#discussion_r4265990759


##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##########
@@ -240,11 +240,8 @@ private void loadDiskBalancerInfo() throws IOException {
       if (diskBalancerInfoFile.exists()) {
         diskBalancerInfo = readDiskBalancerInfoFile(diskBalancerInfoFile);
       } else {
-        boolean shouldRunDefault =
-            conf.getObject(DiskBalancerConfiguration.class)
-                .getDiskBalancerShouldRun();
-        diskBalancerInfo = new DiskBalancerInfo(shouldRunDefault,
-            new DiskBalancerConfiguration());
+        DiskBalancerConfiguration diskBalancerConf = conf.getObject(DiskBalancerConfiguration.class);

Review Comment:
   Thanks @Gargi-jais11. 
   
   Added `@PostConstruct validate()` to `DiskBalancerConfiguration`.
   Added `testInitialDiskBalancerInfoFallsBackToDefaultsForInvalidConfig`, which sets raw invalid values with no `diskBalancer.info` and checks that both the in-memory info and the written YAML use the defaults.



-- 
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

Re: [PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #11503:
URL: https://github.com/apache/ozone/pull/11503#discussion_r4254743663


##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##########
@@ -240,11 +240,8 @@ private void loadDiskBalancerInfo() throws IOException {
       if (diskBalancerInfoFile.exists()) {
         diskBalancerInfo = readDiskBalancerInfoFile(diskBalancerInfoFile);
       } else {
-        boolean shouldRunDefault =
-            conf.getObject(DiskBalancerConfiguration.class)
-                .getDiskBalancerShouldRun();
-        diskBalancerInfo = new DiskBalancerInfo(shouldRunDefault,
-            new DiskBalancerConfiguration());
+        DiskBalancerConfiguration diskBalancerConf = conf.getObject(DiskBalancerConfiguration.class);

Review Comment:
   `getObject `injects fields without calling the validating setters. With this patch, **applyDiskBalancerInfo → toConfiguration()** now runs setThreshold / setDiskBandwidthInMB / setParallelThread on raw XML values. 
   For example, `hdds.datanode.disk.balancer.parallel.thread=0` throws IllegalArgumentException("parallelThread must be a value larger than 0."). Nothing between the OzoneContainer constructor and HddsDatanodeService.main() catches it, so a new datanode exits . Datanodes with an existing diskBalancer.info still start fine.
   
   Could we add a` @PostConstruct validate()` to DiskBalancerConfiguration, the same pattern as `ContainerScannerConfiguration#validate?` It would WARN naming the key and reset threshold, bandwidth and thread count to their defaults. Please also add a test that a bad XML value on first init falls back to the default instead of failing.



-- 
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

Re: [PR] HDDS-16965. Initialize disk balancer state from configured values when diskBalancer.info is absent [ozone]

Posted by "shuan1026 (via GitHub)" <gi...@apache.org>.
shuan1026 commented on code in PR #11503:
URL: https://github.com/apache/ozone/pull/11503#discussion_r4303388634


##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerConfiguration.java:
##########
@@ -169,6 +181,28 @@ public DiskBalancerConfiguration(Double threshold,
   public DiskBalancerConfiguration() {
   }
 
+  @PostConstruct
+  public void validate() {
+    if (threshold <= 0d || threshold >= 100d) {

Review Comment:
   Thanks for the review @slfan1989! 
   Done, NaN is now rejected in both `validate()` and `setThreshold()`, with tests added.



-- 
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