HDDS-15172. Cleanup logging in Iceberg RewriteTablePath action and add validation tests by slfan1989 · Pull Request #10187 · apache/ozone

slfan1989 · 2026-05-05T09:35:55Z

What changes were proposed in this pull request?

This PR improves validation coverage for the Ozone Iceberg RewriteTablePath action and replaces direct console output with SLF4J logging.

The Ozone Iceberg RewriteTablePathOzoneAction already validates several user-provided inputs, such as source/target prefixes and metadata version names. However, some validation paths were not covered by unit tests. In particular, executing the action without configuring the source and target prefixes previously resulted in a raw JVM NullPointerException message instead of the existing validation helper's clearer error message.

This PR proposes the following changes:

Add unit tests for invalid RewriteTablePathOzoneAction inputs:
- missing source/target location prefix
- identical source and target prefixes
- unknown start metadata version
- unknown end metadata version
Add explicit validation for missing source and target prefixes in execute().
Replace a direct System.out.println call in RewriteTablePathOzoneAction with SLF4J logging.

These changes make the Iceberg rewrite path action easier to validate and keep logging consistent with the rest of the Ozone codebase.

What is the link to the Apache JIRA

HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action.

How was this patch tested?

Tested with:

mvn -pl hadoop-ozone/iceberg test

Result:

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.ozone.iceberg.TestRewriteTablePathOzoneAction
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.152 s -- in org.apache.hadoop.ozone.iceberg.TestRewriteTablePathOzoneAction
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

sreejasahithi

Thanks @slfan1989 for adding the additional tests

sreejasahithi · 2026-05-05T09:54:01Z

+  @Test
+  void executeRejectsMissingLocationPrefix() {
+    NullPointerException exception = assertThrows(NullPointerException.class,
+        () -> new RewriteTablePathOzoneAction(table)
+            .stagingLocation(stagingDir.toString() + "/")
+            .execute());
+
+    assertEquals("Source prefix is null", exception.getMessage());
+  }


This does not need to be checked because when we add the CLI HDDS-14946 the source and target prefix would be required fields so they can not be missing.
And once the CLI is added , the source and target prefix will be set before calling the action.
rewriteLocationPrefix is always called by the CLI before execute(), then sourcePrefix will never be null in practice, and the test adds no real value to it.

Updating my earlier comment: while the CLI will enforce that source and target prefixes are set, I think it would be fine to keep this null check and the test as a defensive safeguard. It protects against misuse outside the CLI path and makes the contract of execute() clearer.

@sreejasahithi That makes sense. I kept this as a defensive check for possible non-CLI callers, and to make the execute() contract a bit clearer.

Can you add case to cover the target prefix null case too.

Added a test case to cover the null target prefix case.

sreejasahithi · 2026-05-05T10:09:01Z

    String stagingPath = RewriteTablePathUtil.stagingPath(versionFilePath, sourcePrefix, stagingDir);

-    System.out.println("Processing version file " + versionFilePath);
+    LOG.info("Processing version file {}", versionFilePath);


LOG.info will cause noise in the command output with the additional information as follows

2026-05-05 12:00:01,000 [main] INFO RewriteTablePathOzoneAction: Processing version file v4.metadata.json 2026-05-05 12:00:01,050 [main] INFO RewriteTablePathOzoneAction: Processing version file v3.metadata.json

Thanks for pointing this out. I will change this to DEBUG so it does not add noise to the default command output, while still keeping the information available when debug logging is enabled.

slfan1989 · 2026-05-06T23:07:23Z

@adoroszlai @ashishkumar50 Could you please help review this PR? Thank you very much!

slfan1989 · 2026-05-06T23:08:16Z

@sreejasahithi Do you have any other suggestions?

sreejasahithi

Thanks @slfan1989 for updating the patch

sreejasahithi · 2026-05-11T14:41:14Z

You could also add a case like follows where the version file is missing.

void startVersionRejectsDeletedVersionFile() { List<String> metadataPaths = metadataLogEntryPaths(table); String existingName = RewriteTablePathUtil.fileName(metadataPaths.get(0)); // delete the physical file so it's in the log but missing on disk table.io().deleteFile(metadataPaths.get(0)); IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> new RewriteTablePathOzoneAction(table) .rewriteLocationPrefix(sourcePrefix, targetPrefix) .startVersion(existingName) .execute()); assertTrue(exception.getMessage().contains("does not exist")); }

assertTrue(exception.getMessage().contains

Please use assertThat(exception).hasMessageContaining instead.

@sreejasahithi @adoroszlai Added a test case for the scenario where the requested start version exists in the metadata log but the physical metadata file has been deleted. The test verifies that the action fails with an IllegalArgumentException indicating that the version file does not exist.

sreejasahithi · 2026-05-12T11:05:14Z

+  void startVersionRejectsDeletedVersionFile() {
+    List<String> metadataPaths = metadataLogEntryPaths(table);


Sorry my previous comment was not clear enough, could you please also do for end version.

@sreejasahithi Added the corresponding end version test case where the version exists in the metadata log but the physical metadata file is missing.

…g RewriteTablePath action.

sreejasahithi

Thanks @slfan1989
LGTM

slfan1989 · 2026-05-18T11:01:52Z

Thanks @slfan1989
LGTM

@sreejasahithi Thanks for the review! @adoroszlai @ashishkumar50 Could you please take another look when you get a chance? Thank you!

adoroszlai

Thanks @slfan1989 for updating the patch, LGTM

ashishkumar50

LGTM

adoroszlai · 2026-05-18T11:49:48Z

Thanks @slfan1989 for the patch, @ashishkumar50, @sreejasahithi for the review.

slfan1989 · 2026-05-18T13:24:03Z

Thanks @slfan1989 for the patch, @ashishkumar50, @sreejasahithi for the review.

@adoroszlai Thanks a lot for your review and for merging this PR! @ashishkumar50 @sreejasahithi Thanks a lot for your review!

sreejasahithi reviewed May 5, 2026

View reviewed changes

slfan1989 requested a review from sreejasahithi May 6, 2026 23:07

sreejasahithi reviewed May 11, 2026

View reviewed changes

sreejasahithi reviewed May 12, 2026

View reviewed changes

slfan1989 added 4 commits May 15, 2026 15:49

HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…

f2c5111

…g RewriteTablePath action.

HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…

bb6d12e

…g RewriteTablePath action.

HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…

cb99c0d

…g RewriteTablePath action.

HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…

a30d297

…g RewriteTablePath action.

slfan1989 force-pushed the HDDS-15172 branch from 791d6dd to a30d297 Compare May 15, 2026 07:54

Merge branch 'apache:master' into HDDS-15172

e49b758

adoroszlai requested a review from sreejasahithi May 16, 2026 17:11

sreejasahithi approved these changes May 16, 2026

View reviewed changes

Merge branch 'master' into HDDS-15172

8f8b287

adoroszlai reviewed May 18, 2026

View reviewed changes

adoroszlai changed the title ~~HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action.~~ HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action May 18, 2026

adoroszlai changed the title ~~HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action~~ HDDS-15172. Cleanup logging in Iceberg RewriteTablePath action and add validation tests May 18, 2026

ashishkumar50 approved these changes May 18, 2026

View reviewed changes

adoroszlai merged commit b2f63c9 into apache:master May 18, 2026
49 checks passed

		void startVersionRejectsDeletedVersionFile() {
		List<String> metadataPaths = metadataLogEntryPaths(table);

Uh oh!

Conversation

slfan1989 commented May 5, 2026 • 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!

sreejasahithi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreejasahithi May 5, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slfan1989 commented May 6, 2026

Uh oh!

slfan1989 commented May 6, 2026

Uh oh!

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

sreejasahithi left a comment

Choose a reason for hiding this comment

Uh oh!

slfan1989 commented May 18, 2026

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adoroszlai commented May 18, 2026

Uh oh!

slfan1989 commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

slfan1989 commented May 5, 2026 •

edited

Loading

sreejasahithi May 5, 2026 •

edited

Loading