Skip to content

HDDS-15172. Cleanup logging in Iceberg RewriteTablePath action and add validation tests #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

Merged
adoroszlai merged 6 commits into from
May 18, 2026
Merged

HDDS-15172. Cleanup logging in Iceberg RewriteTablePath action and add validation tests #10187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f2c5111
HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…
slfan1989 May 5, 2026
bb6d12e
HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…
slfan1989 May 6, 2026
cb99c0d
HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…
slfan1989 May 12, 2026
a30d297
HDDS-15172. Add validation tests and logging cleanup for Ozone Iceber…
slfan1989 May 13, 2026
e49b758
Merge branch 'apache:master' into HDDS-15172
slfan1989 May 16, 2026
8f8b287
Merge branch 'master' into HDDS-15172
slfan1989 May 18, 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
5 changes: 4 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 @@ -158,6 +158,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 @@ -289,7 +292,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.debug("Processing version file {}", versionFilePath);
TableMetadata newTableMetadata = RewriteTablePathUtil.replacePaths(metadata, sourcePrefix, targetPrefix);
TableMetadataParser.overwrite(newTableMetadata, table.io().newOutputFile(stagingPath));

Expand Down
83 changes: 83 additions & 0 deletions ...ceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java
View file
Open in desktop

@sreejasahithi sreejasahithi May 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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"));
  }

@adoroszlai adoroszlai May 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

assertTrue(exception.getMessage().contains

Please use assertThat(exception).hasMessageContaining instead.

@slfan1989 slfan1989 May 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,89 @@ void tablePathRewriteForStartAndEndVersionProvided() throws Exception {
}

@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 +224 to +232

@sreejasahithi sreejasahithi May 5, 2026

Copy link
Copy Markdown
Contributor

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.

@sreejasahithi sreejasahithi May 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@slfan1989 slfan1989 May 6, 2026

Copy link
Copy Markdown
Contributor Author

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.

@ashishkumar50 ashishkumar50 May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@slfan1989 slfan1989 May 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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


@Test
void executeRejectsMissingTargetPrefix() {
NullPointerException exception = assertThrows(NullPointerException.class,
() -> new RewriteTablePathOzoneAction(table)
.rewriteLocationPrefix(sourcePrefix, null));

assertEquals("Target prefix is null", exception.getMessage());
}

@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 startVersionRejectsDeletedVersionFile() {
List<String> metadataPaths = metadataLogEntryPaths(table);
Comment on lines +267 to +268

@sreejasahithi sreejasahithi May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@slfan1989 slfan1989 May 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

String existingName = RewriteTablePathUtil.fileName(metadataPaths.get(0));
table.io().deleteFile(metadataPaths.get(0));

IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> new RewriteTablePathOzoneAction(table)
.rewriteLocationPrefix(sourcePrefix, targetPrefix)
.startVersion(existingName)
.execute());

assertThat(exception).hasMessageContaining("does not exist");
}

@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());
}

@Test
void endVersionRejectsDeletedVersionFile() {
List<String> metadataPaths = metadataLogEntryPaths(table);
String existingName = RewriteTablePathUtil.fileName(metadataPaths.get(0));
table.io().deleteFile(metadataPaths.get(0));

IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> new RewriteTablePathOzoneAction(table)
.rewriteLocationPrefix(sourcePrefix, targetPrefix)
.endVersion(existingName)
.execute());

assertThat(exception).hasMessageContaining("does not exist");
}

void defaultStagingDirIsUnderTableMetadataLocation() {
String metadataLocation = RewriteTablePathOzoneUtils.getMetadataLocation(table);

Expand Down