Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-18 Thread via GitHub 


slfan1989 commented on PR #10187:
URL: https://github.com/apache/ozone/pull/10187#issuecomment-4477009818

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


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-12 Thread via GitHub 


slfan1989 commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3230955652


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +190,75 @@ 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());
+  }
+
+  @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 metadataPaths = metadataLogEntryPaths(table);

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



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-12 Thread via GitHub 


sreejasahithi commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3225848701


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +190,75 @@ 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());
+  }
+
+  @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 metadataPaths = metadataLogEntryPaths(table);

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



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-12 Thread via GitHub 


slfan1989 commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3224651708


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##


Review Comment:
   @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.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-12 Thread via GitHub 


slfan1989 commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3224644955


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

Review Comment:
   Added a test case to cover the null target prefix case.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-11 Thread via GitHub 


ashishkumar50 commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3224247221


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

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



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-11 Thread via GitHub 


adoroszlai commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3219862845


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##


Review Comment:
   > `assertTrue(exception.getMessage().contains`
   
   Please use `assertThat(exception).hasMessageContaining` instead.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-11 Thread via GitHub 


sreejasahithi commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3219783261


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##


Review Comment:
   You could also add a case like follows where the version file is missing. 
   
   ```
 void startVersionRejectsDeletedVersionFile() {
   List 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"));
 }
   ```



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-06 Thread via GitHub 


slfan1989 commented on PR #10187:
URL: https://github.com/apache/ozone/pull/10187#issuecomment-4392862707

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


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-06 Thread via GitHub 


slfan1989 commented on PR #10187:
URL: https://github.com/apache/ozone/pull/10187#issuecomment-4392866269

   @sreejasahithi Do you have any other suggestions?


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-05 Thread via GitHub 


slfan1989 commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3192753657


##
hadoop-ozone/iceberg/src/main/java/org/apache/hadoop/ozone/iceberg/RewriteTablePathOzoneAction.java:
##
@@ -241,7 +249,7 @@ private Set> 
rewriteVersionFile(TableMetadata metadata, Str
 Set> result = new HashSet<>();
 String stagingPath = RewriteTablePathUtil.stagingPath(versionFilePath, 
sourcePrefix, stagingDir);
 
-System.out.println("Processing version file " + versionFilePath);
+LOG.info("Processing version file {}", versionFilePath);

Review Comment:
   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.
   



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-05 Thread via GitHub 


slfan1989 commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3192731980


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

Review Comment:
   @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.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-05 Thread via GitHub 


sreejasahithi commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3187953205


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

Review Comment:
   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.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-05 Thread via GitHub 


sreejasahithi commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3187953205


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

Review Comment:
   Updating my earlier comment: while the CLI will enforce that source and 
target prefixes are set, I think it still makes sense 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.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-05 Thread via GitHub 


sreejasahithi commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3187953205


##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

Review Comment:
   But it should not be a problem keeping this check and test for it, it would 
be more like a defensive check so should be good.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] HDDS-15172. Add validation tests and logging cleanup for Ozone Iceberg RewriteTablePath action. [ozone]

2026-05-05 Thread via GitHub 


sreejasahithi commented on code in PR #10187:
URL: https://github.com/apache/ozone/pull/10187#discussion_r3187617204


##
hadoop-ozone/iceberg/src/main/java/org/apache/hadoop/ozone/iceberg/RewriteTablePathOzoneAction.java:
##
@@ -241,7 +249,7 @@ private Set> 
rewriteVersionFile(TableMetadata metadata, Str
 Set> result = new HashSet<>();
 String stagingPath = RewriteTablePathUtil.stagingPath(versionFilePath, 
sourcePrefix, stagingDir);
 
-System.out.println("Processing version file " + versionFilePath);
+LOG.info("Processing version file {}", versionFilePath);

Review Comment:
   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
   ```



##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -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());
+  }

Review Comment:
   This does not need to be checked because when we add the CLI 
[HDDS-14946](https://issues.apache.org/jira/browse/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.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]