Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
slfan1989 wants to merge 2 commits into
base: master
Choose a base branch
from
+55 −1
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30a02b7
HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…
slfan1989 May 5, 2026
7e7a762
HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…
slfan1989 May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion ...ne/iceberg/src/main/java/org/apache/hadoop/ozone/iceberg/RewriteTablePathOzoneAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import org.apache.iceberg.actions.ImmutableRewriteTablePath;
import org.apache.iceberg.actions.RewriteTablePath;
import org.apache.iceberg.util.Pair;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* An implementation of {@link RewriteTablePath} for Apache Ozone backed Iceberg tables.
Expand All @@ -50,6 +52,9 @@
*/
public class RewriteTablePathOzoneAction implements RewriteTablePath {

private static final Logger LOG =
LoggerFactory.getLogger(RewriteTablePathOzoneAction.class);

private String sourcePrefix;
private String targetPrefix;
private String startVersionName;
Expand Down Expand Up @@ -129,6 +134,9 @@ private Result doExecute() {
}

private void validateInputs() {
RewriteTablePathOzoneUtils.checkNonNullNonEmpty(sourcePrefix, "Source prefix");
RewriteTablePathOzoneUtils.checkNonNullNonEmpty(targetPrefix, "Target prefix");

if (sourcePrefix.equals(targetPrefix)) {
throw new IllegalArgumentException(
String.format(
Expand Down Expand Up @@ -241,7 +249,7 @@ private Set<Pair<String, String>> rewriteVersionFile(TableMetadata metadata, Str
Set<Pair<String, String>> result = new HashSet<>();
String stagingPath = RewriteTablePathUtil.stagingPath(versionFilePath, sourcePrefix, stagingDir);

System.out.println("Processing version file " + versionFilePath);
LOG.info("Processing version file {}", versionFilePath);
Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi May 5, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@slfan1989 slfan1989 May 6, 2026

Choose a reason for hiding this comment

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

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.

TableMetadata newTableMetadata = RewriteTablePathUtil.replacePaths(metadata, sourcePrefix, targetPrefix);
TableMetadataParser.overwrite(newTableMetadata, table.io().newOutputFile(stagingPath));

Expand Down
46 changes: 46 additions & 0 deletions ...ceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hadoop.ozone.iceberg;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.BufferedReader;
Expand Down Expand Up @@ -188,6 +189,51 @@ void tablePathRewriteForStartAndEndVersionProvided() throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}

@Test
void executeRejectsMissingLocationPrefix() {
NullPointerException exception = assertThrows(NullPointerException.class,
() -> new RewriteTablePathOzoneAction(table)
.stagingLocation(stagingDir.toString() + "/")
.execute());

assertEquals("Source prefix is null", exception.getMessage());
}
Comment on lines +192 to +200
Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi May 5, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@slfan1989 slfan1989 May 6, 2026

Choose a reason for hiding this comment

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

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


@Test
void rewriteLocationPrefixRejectsSameSourceAndTarget() {
IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> new RewriteTablePathOzoneAction(table)
.rewriteLocationPrefix(sourcePrefix, sourcePrefix)
.execute());

assertEquals("Source prefix cannot be the same as target prefix (" +
sourcePrefix + ")", exception.getMessage());
}

@Test
void startVersionRejectsUnknownVersion() {
IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> new RewriteTablePathOzoneAction(table)
.rewriteLocationPrefix(sourcePrefix, targetPrefix)
.startVersion("missing.metadata.json")
.execute());

assertEquals("Cannot find provided version file missing.metadata.json " +
"in metadata log.", exception.getMessage());
}

@Test
void endVersionRejectsUnknownVersion() {
IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> new RewriteTablePathOzoneAction(table)
.rewriteLocationPrefix(sourcePrefix, targetPrefix)
.endVersion("missing.metadata.json")
.execute());

assertEquals("Cannot find provided version file missing.metadata.json " +
"in metadata log.", exception.getMessage());
}

/**
* For every staged metadata JSON file in the CSV, parses the file and asserts that:
* - The table location starts with target
Expand Down