hudi-agent commented on code in PR #18816:
URL: https://github.com/apache/hudi/pull/18816#discussion_r3290871151
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/RepairsCommand.java:
##########
@@ -201,7 +201,9 @@ public void removeCorruptedPendingCleanAction() {
TimelineUtils.deleteInstantFile(client.getStorage(),
client.getTimelinePath(),
instant, client.getInstantFileNameGenerator());
} catch (IOException ioe) {
- if (ioe.getMessage().contains("Not an Avro data file")) {
+ if (ioe.getMessage() == null || ioe.getMessage().contains("Not an Avro
data file")
Review Comment:
π€ nit: this multi-condition exception-message match is getting hard to read
and brittle. Could you extract a small helper like
`isCorruptedInstantFileException(IOException ioe)` so the intent is obvious at
the call site and easier to extend later?
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestRepairsCommand.java:
##########
@@ -161,7 +156,7 @@ public void testAddPartitionMetaWithDryRun() throws
IOException {
@Test
public void testAddPartitionMetaWithRealRun() throws IOException {
// create commit instant
- Files.createFile(Paths.get(tablePath, ".hoodie", "100.commit"));
+ Files.createFile(Paths.get(tablePath, ".hoodie/timeline/", "100.commit"));
Review Comment:
π€ nit: hardcoding `".hoodie/timeline/"` here is fragile if the layout
changes again. Could you use `metaClient.getTimelinePath()` like the other
updated call sites in this file do?
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestTableCommand.java:
##########
@@ -136,11 +136,11 @@ public void testDefaultCreate() {
// Test meta
HoodieTableMetaClient client = HoodieCLI.getTableMetaClient();
- assertEquals(archivePath, client.getArchivePath());
+ assertEquals(archivePath, client.getArchivePath().toString());
assertEquals(tablePath, client.getBasePath().toString());
assertEquals(metaPath, client.getMetaPath().toString());
assertEquals(HoodieTableType.COPY_ON_WRITE, client.getTableType());
- assertEquals(new Integer(1),
client.getTimelineLayoutVersion().getVersion());
+
assertEquals(org.apache.hudi.common.table.timeline.versioning.TimelineLayoutVersion.CURR_VERSION,
client.getTimelineLayoutVersion().getVersion());
Review Comment:
π€ nit: could you add a static import (or regular import) for
`TimelineLayoutVersion.CURR_VERSION` instead of using the fully-qualified name
inline?
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestRepairsCommand.java:
##########
@@ -257,16 +264,16 @@ public void testRemoveCorruptedPendingCleanAction()
throws IOException {
for (int i = 100; i < 104; i++) {
String timestamp = String.valueOf(i);
// Write corrupted requested Clean File
-
HoodieTestCommitMetadataGenerator.createEmptyCleanRequestedFile(tablePath,
timestamp, conf);
+ org.apache.hadoop.fs.Path filePath = new
org.apache.hadoop.fs.Path(metaClient.getTimelinePath() + "/" + timestamp +
".clean.requested");
Review Comment:
π€ nit: could you add imports for `org.apache.hadoop.fs.Path`,
`HoodieTestDataGenerator`, `java.util.HashSet`, and
`java.util.stream.Collectors` rather than fully-qualifying them inline? It's
used in several spots now and hurts readability.
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
--
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]
