voonhous commented on code in PR #19877:
URL: https://github.com/apache/hudi/pull/19877#discussion_r3975345332


##########
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestCompactionCommand.java:
##########
@@ -146,6 +173,203 @@ public void testCompactionShow() throws IOException {
     assertNotNull(result);
   }
 
+  /**
+   * Test case of the compaction validation entry point of {@link SparkMain}, 
which the
+   * 'compaction validate' command reaches through a spark-submit of its own.
+   */
+  @Test
+  public void testSparkMainCompactValidate() throws Exception {
+    createPendingCompactions();
+    String outputPath = outputPath("validate");
+
+    SparkMain.doCompactValidate(jsc(), tablePath, PENDING_COMPACTION_INSTANT, 
outputPath, 2);
+
+    List<ValidationOpResult> results = readOperationResults(outputPath);
+    assertEquals(operationsOf(PENDING_COMPACTION_INSTANT).size(), 
results.size());
+    assertTrue(results.stream().allMatch(ValidationOpResult::isSuccess), 
results.toString());
+    assertEquals(fileIdsOf(PENDING_COMPACTION_INSTANT),
+        results.stream().map(result -> 
result.getOperation().getFileId()).collect(Collectors.toSet()));
+  }
+
+  @Test
+  public void testSparkMainCompactValidateReportsMissingLogFile() throws 
Exception {
+    createPendingCompactions();
+    HoodieCompactionOperation broken = 
operationsOf(PENDING_COMPACTION_INSTANT).get(0);
+    // a log file the plan reads is gone, so that operation can no longer be 
compacted
+    Files.delete(Paths.get(tablePath, broken.getPartitionPath(), 
broken.getDeltaFilePaths().get(0)));
+    String outputPath = outputPath("validate-broken");
+
+    SparkMain.doCompactValidate(jsc(), tablePath, PENDING_COMPACTION_INSTANT, 
outputPath, 2);
+
+    List<ValidationOpResult> results = readOperationResults(outputPath);
+    assertEquals(operationsOf(PENDING_COMPACTION_INSTANT).size(), 
results.size());
+    List<ValidationOpResult> failed = results.stream().filter(result -> 
!result.isSuccess()).collect(Collectors.toList());
+    assertEquals(1, failed.size(), results.toString());
+    assertEquals(broken.getFileId(), failed.get(0).getOperation().getFileId());
+    assertTrue(failed.get(0).getException().isPresent());
+  }
+
+  /**
+   * Repair only validates the plan and reports the renames it would need; 
with the plan intact
+   * there is nothing to rename and the plan is left alone, whether or not 
this is a dry run.
+   */
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  public void testSparkMainCompactRepair(boolean dryRun) throws Exception {
+    createPendingCompactions();
+    Set<String> fileIdsBefore = fileIdsOf(PENDING_COMPACTION_INSTANT);
+    String outputPath = outputPath("repair-" + dryRun);
+
+    SparkMain.doCompactRepair(jsc(), tablePath, PENDING_COMPACTION_INSTANT, 
outputPath, 2, dryRun);
+
+    assertTrue(readOperationResults(outputPath).isEmpty());
+    
assertTrue(pendingCompactionInstants().contains(PENDING_COMPACTION_INSTANT));
+    assertEquals(fileIdsBefore, fileIdsOf(PENDING_COMPACTION_INSTANT));
+  }
+
+  /**
+   * Unscheduling a plan takes the requested compaction instant off the 
timeline, unless this is a
+   * dry run. The other pending plans are left alone either way.
+   */
+  @ParameterizedTest
+  @CsvSource({"true, true", "true, false", "false, true", "false, false"})

Review Comment:
   Done: both are now `@ValueSource` over `dryRun` alone with `skipValidation` 
fixed, and the javadoc says why. Done in 513e0a1d8f83.



##########
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestMetadataCommand.java:
##########
@@ -224,6 +235,222 @@ public void testGetRecordIndexInfoForPartitionedRLI() 
throws Exception {
     }
   }
 
+  @Test
+  public void testMetadataStatsAndFileListing() throws Exception {
+    writeOneCommit(true);
+    connectToTable();
+
+    // The command opens the reader with metadata metrics off, so there is 
nothing to report on,
+    // but the stat table is still rendered.
+    Object stats = shell.evaluate(() -> "metadata stats");
+    assertTrue(ShellEvaluationResultUtil.isSuccess(stats));
+    assertTrue(stats.toString().contains("stat key"), stats.toString());
+    assertTrue(renderedRows(stats.toString()).isEmpty(), stats.toString());
+
+    Object partitions = shell.evaluate(() -> "metadata list-partitions");
+    assertTrue(ShellEvaluationResultUtil.isSuccess(partitions));
+    Set<String> written = writtenPartitions();
+    assertFalse(written.isEmpty());
+    assertEquals(written, renderedRows(partitions.toString()).stream()
+        .map(row -> row.get(0)).collect(Collectors.toSet()));
+
+    // The files of one partition, as the metadata table has them.
+    Object files = shell.evaluate(() -> "metadata list-files --partition " + 
DEFAULT_FIRST_PARTITION_PATH);
+    assertTrue(ShellEvaluationResultUtil.isSuccess(files));
+    Set<String> baseFiles = baseFilesOf(DEFAULT_FIRST_PARTITION_PATH);
+    assertFalse(baseFiles.isEmpty());
+    assertEquals(baseFiles.size(), renderedRows(files.toString()).size(), 
files.toString());
+    for (String baseFile : baseFiles) {
+      assertTrue(files.toString().contains(baseFile), files.toString());
+    }
+
+    // Without a partition the base path itself is listed, which holds no data 
files.

Review Comment:
   Confirmed: `fetchAllFilesInPartition` turns the base path into the 
`NON_PARTITIONED_NAME` key, which a partitioned table's files index has no 
record for, so the lookup misses. Comment reworded to say that. Done in 
513e0a1d8f83.



-- 
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]

Reply via email to