HDDS-15290. DiskBalancer should write info file atomically. by slfan1989 · Pull Request #10284 · apache/ozone

slfan1989 · 2026-05-16T04:44:22Z

What changes were proposed in this pull request?

This PR updates DiskBalancer to persist diskBalancer.info atomically.

Previously, DiskBalancer overwrote diskBalancer.info by deleting the existing file and creating a new one. If the write failed between these steps, the existing configuration could be lost or left incomplete.

This patch changes the persistence flow to:

create the parent directory if needed;
write the new DiskBalancerInfo to a temporary file in the same directory;
atomically move the temporary file to diskBalancer.info;
preserve the existing file if the temporary write fails;
provide clear IOException messages for directory creation, temporary file creation, and overwrite failures.

This makes DiskBalancer info persistence more robust and avoids losing the previous valid configuration on write failure.

What is the link to the Apache JIRA

JIRA: HDDS-15290. DiskBalancer should write info file atomically.

How was this patch tested?

Added unit tests.

adoroszlai

Thanks @slfan1989 for the patch.

adoroszlai · 2026-05-16T08:41:09Z

+    final Path tempFile;
+    try {
+      tempFile = Files.createTempFile(parent, path.getName(), ".tmp");
+    } catch (IOException e) {
+      throw new IOException(
+          "Unable to create temporary DiskBalancerInfo file under: "
+              + parent, e);
+    }
+    boolean moved = false;
+    try {
+      writer.write(diskBalancerInfo, tempFile.toFile());
+      try {
+        Files.move(tempFile, target, StandardCopyOption.ATOMIC_MOVE,
+            StandardCopyOption.REPLACE_EXISTING);
+      } catch (IOException e) {
+        throw new IOException(
+            "Unable to overwrite the DiskBalancerInfo file: " + target, e);
      }


Temp file and atomic move to final location is not needed, because YamlUtils uses AtomicFileOutputStream internally, which performs the same.

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerYaml.java

Lines 59 to 60 in 003b26a

final DiskBalancerInfoYaml data = getDiskBalancerInfoYaml(diskBalancerInfo);

YamlUtils.dump(yaml, data, path, LOG);

ozone/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/YamlUtils.java

Lines 55 to 56 in 003b26a

public static void dump(Yaml yaml, Object data, File file, Logger log) throws IOException {

try (OutputStream out = new AtomicFileOutputStream(file);

Updated. Removed the redundant temp-file and atomic move wrapper. DiskBalancerService now only creates the parent directory and relies on DiskBalancerYaml/YamlUtils, which already uses AtomicFileOutputStream for atomic writes.

adoroszlai · 2026-05-16T08:45:21Z

+    assertTrue(exception.getMessage()
+        .startsWith("Unable to overwrite the DiskBalancerInfo file: "));


nit: please replace with assertThat(exception).hasMessageStartingWith(...) (see HDDS-9951)

Updated. Replaced the message prefix assertions with assertThat(exception).hasMessageStartingWith(...).

peterxcli · 2026-05-16T09:25:52Z

+  }
+
+  @VisibleForTesting
+  static void writeDiskBalancerInfoFile(DiskBalancerInfo diskBalancerInfo,


write the new DiskBalancerInfo to a temporary file in the same directory;

atomically move the temporary file to diskBalancer.info;

Curious where the two atomic write steps are implemented in this change?

The two steps are implemented by AtomicFileOutputStream used in YamlUtils.dump():

DiskBalancerService.writeDiskBalancerInfoFile()
-> DiskBalancerYaml.createDiskBalancerInfoFile()
-> YamlUtils.dump()
-> new AtomicFileOutputStream(file)

So DiskBalancerService no longer creates the temporary file or moves it directly. It relies on the existing YamlUtils atomic write path and avoids the previous non-atomic delete/createNewFile before writing.

so this pr is only for improving the exception handling for disk balancer info file atomic write?

Not only exception handling. The main purpose is to remove the previous non-atomic delete/createNewFile step before writing diskBalancer.info.

The actual atomic write is already handled by DiskBalancerYaml -> YamlUtils.dump() -> AtomicFileOutputStream. This PR keeps that path, creates the parent directory when needed, and wraps directory/write failures with clearer error messages.

I think the important part is removal of explicit delete and createNewFile from here:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java

Line 336 in 24a61fe

if (!path.delete() || !path.createNewFile()) {

oh, I got it, then how about just remove that branch and add some exception handling around those called functions that might throw exception? looks like we dont need to add an additional static func for this?

That makes sense.

Since YamlUtils already provides the atomic write behavior, a separate static helper is not strictly needed.

I will simplify the change by keeping the logic inside writeDiskBalancerInfoTo(), remove the extra static method, and only add parent directory creation plus clearer exception wrapping around the existing DiskBalancerYaml.createDiskBalancerInfoFile() call.

…ozone/container/diskbalancer/DiskBalancerService.java Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

peterxcli

LGTM +1

adoroszlai · 2026-05-17T10:33:30Z

@Gargi-jais11 would you like to take a look?

Gargi-jais11 · 2026-05-17T13:12:06Z

@Gargi-jais11 would you like to take a look?

@adoroszlai Yes will take a look. Give me sometime.

slfan1989 added 2 commits May 16, 2026 12:41

HDDS-15290. DiskBalancer should write info file atomically.

c7f6ad0

HDDS-15290. DiskBalancer should write info file atomically.

71eb216

adoroszlai reviewed May 16, 2026

View reviewed changes

HDDS-15290. DiskBalancer should write info file atomically.

658192c

peterxcli reviewed May 16, 2026

View reviewed changes

slfan1989 and others added 2 commits May 16, 2026 17:38

Update hadoop-hdds/container-service/src/main/java/org/apache/hadoop/…

e37864b

…ozone/container/diskbalancer/DiskBalancerService.java Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

HDDS-15290. DiskBalancer should write info file atomically.

9f5565f

adoroszlai requested a review from peterxcli May 17, 2026 05:36

peterxcli approved these changes May 17, 2026

View reviewed changes

	final DiskBalancerInfoYaml data = getDiskBalancerInfoYaml(diskBalancerInfo);
	YamlUtils.dump(yaml, data, path, LOG);

	public static void dump(Yaml yaml, Object data, File file, Logger log) throws IOException {
	try (OutputStream out = new AtomicFileOutputStream(file);

		assertTrue(exception.getMessage()
		.startsWith("Unable to overwrite the DiskBalancerInfo file: "));

Conversation

slfan1989 commented May 16, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

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

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!

peterxcli left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented May 17, 2026

Uh oh!

Gargi-jais11 commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants