yihua commented on code in PR #13007:
URL: https://github.com/apache/hudi/pull/13007#discussion_r2017722269
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/table/functional/TestHoodieSparkMergeOnReadTableRollback.java:
##########
@@ -242,8 +261,31 @@ void testRollbackWithDeltaAndCompactionCommit(boolean
rollbackUsingMarkers, int
// Verify there are no errors
assertNoWriteErrors(statuses);
+ WriteMarkers markers =
WriteMarkersFactory.get(secondCfg.getMarkersType(), hoodieTable, commitTime1);
+ IOType logFileIOType = tableVersion >= 8 ? IOType.CREATE :
IOType.APPEND;
+ assertTrue(markers.allMarkerFilePaths().stream().anyMatch(name ->
name.contains("log") && name.contains(logFileIOType.name())));
+
// Test failed delta commit rollback
secondClient.rollback(commitTime1);
+ // After rollback for table version 6, there should be new
Review Comment:
The comment does not parse to me.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/table/functional/TestHoodieSparkMergeOnReadTableRollback.java:
##########
@@ -1029,4 +1179,37 @@ private HoodieWriteConfig getWriteConfig(boolean
autoCommit, boolean rollbackUsi
.build());
return cfgBuilder.build();
}
+
+ /**
+ * Scenario: data table is updated, no changes to MDT
+ */
+ @Test
+ public void testRollbackWithFailurePreMDT() throws Exception {
Review Comment:
Are these functional tests necessary, i.e., what additional coverage do
these tests provide?
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/table/functional/TestHoodieSparkMergeOnReadTableRollback.java:
##########
@@ -242,8 +261,31 @@ void testRollbackWithDeltaAndCompactionCommit(boolean
rollbackUsingMarkers, int
// Verify there are no errors
assertNoWriteErrors(statuses);
+ WriteMarkers markers =
WriteMarkersFactory.get(secondCfg.getMarkersType(), hoodieTable, commitTime1);
+ IOType logFileIOType = tableVersion >= 8 ? IOType.CREATE :
IOType.APPEND;
+ assertTrue(markers.allMarkerFilePaths().stream().anyMatch(name ->
name.contains("log") && name.contains(logFileIOType.name())));
+
// Test failed delta commit rollback
secondClient.rollback(commitTime1);
+ // After rollback for table version 6, there should be new
+ metaClient = HoodieTableMetaClient.reload(metaClient);
+ HoodieInstant rollbackInstant =
metaClient.getActiveTimeline().getRollbackTimeline().firstInstant().get();
+ HoodieRollbackMetadata rollbackMetadata =
metaClient.getActiveTimeline().readInstantContent(rollbackInstant,
HoodieRollbackMetadata.class);
+ List<StoragePathInfo> logFiles = listAllLogFilesInPath(hoodieTable);
+ if (tableVersion < HoodieTableVersion.EIGHT.versionCode()) {
+ // Ensure rollback metadata contains rollback log files for table
version 6
+ assertTrue(rollbackMetadata.getPartitionMetadata().values().stream()
+ .noneMatch(rollbackPartitionMetadata ->
rollbackPartitionMetadata.getRollbackLogFiles().isEmpty()));
+ // Total 3 partitions would contain 6 log files - one rollback log
file and one log file with data block
+ assertEquals(6, logFiles.size());
+ } else {
+ // Ensure rollback metadata does not contain rollback log files for
table version 8
+ assertFalse(rollbackMetadata.getPartitionMetadata().values().stream()
+ .noneMatch(rollbackPartitionMetadata ->
rollbackPartitionMetadata.getRollbackLogFiles().isEmpty()));
Review Comment:
Validate that rollback metadata contains log files in the deleted files?
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/table/functional/TestHoodieSparkMergeOnReadTableRollback.java:
##########
@@ -242,8 +261,31 @@ void testRollbackWithDeltaAndCompactionCommit(boolean
rollbackUsingMarkers, int
// Verify there are no errors
assertNoWriteErrors(statuses);
+ WriteMarkers markers =
WriteMarkersFactory.get(secondCfg.getMarkersType(), hoodieTable, commitTime1);
+ IOType logFileIOType = tableVersion >= 8 ? IOType.CREATE :
IOType.APPEND;
+ assertTrue(markers.allMarkerFilePaths().stream().anyMatch(name ->
name.contains("log") && name.contains(logFileIOType.name())));
+
// Test failed delta commit rollback
secondClient.rollback(commitTime1);
+ // After rollback for table version 6, there should be new
+ metaClient = HoodieTableMetaClient.reload(metaClient);
+ HoodieInstant rollbackInstant =
metaClient.getActiveTimeline().getRollbackTimeline().firstInstant().get();
+ HoodieRollbackMetadata rollbackMetadata =
metaClient.getActiveTimeline().readInstantContent(rollbackInstant,
HoodieRollbackMetadata.class);
+ List<StoragePathInfo> logFiles = listAllLogFilesInPath(hoodieTable);
+ if (tableVersion < HoodieTableVersion.EIGHT.versionCode()) {
+ // Ensure rollback metadata contains rollback log files for table
version 6
+ assertTrue(rollbackMetadata.getPartitionMetadata().values().stream()
+ .noneMatch(rollbackPartitionMetadata ->
rollbackPartitionMetadata.getRollbackLogFiles().isEmpty()));
+ // Total 3 partitions would contain 6 log files - one rollback log
file and one log file with data block
+ assertEquals(6, logFiles.size());
+ } else {
+ // Ensure rollback metadata does not contain rollback log files for
table version 8
+ assertFalse(rollbackMetadata.getPartitionMetadata().values().stream()
+ .noneMatch(rollbackPartitionMetadata ->
rollbackPartitionMetadata.getRollbackLogFiles().isEmpty()));
+ // After rollback log files should be deleted
+ assertEquals(0, logFiles.size());
+ }
+
allFiles = listAllBaseFilesInPath(hoodieTable);
// After rollback, there should be no base file with the failed commit
time
List<String> remainingFiles = allFiles.stream()
Review Comment:
Instead, we should validate in the functional tests that log files written
by failed commit are deleted in table version 8.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/table/functional/TestHoodieSparkMergeOnReadTableRollback.java:
##########
@@ -242,8 +261,31 @@ void testRollbackWithDeltaAndCompactionCommit(boolean
rollbackUsingMarkers, int
// Verify there are no errors
assertNoWriteErrors(statuses);
+ WriteMarkers markers =
WriteMarkersFactory.get(secondCfg.getMarkersType(), hoodieTable, commitTime1);
+ IOType logFileIOType = tableVersion >= 8 ? IOType.CREATE :
IOType.APPEND;
+ assertTrue(markers.allMarkerFilePaths().stream().anyMatch(name ->
name.contains("log") && name.contains(logFileIOType.name())));
+
// Test failed delta commit rollback
secondClient.rollback(commitTime1);
+ // After rollback for table version 6, there should be new
+ metaClient = HoodieTableMetaClient.reload(metaClient);
+ HoodieInstant rollbackInstant =
metaClient.getActiveTimeline().getRollbackTimeline().firstInstant().get();
+ HoodieRollbackMetadata rollbackMetadata =
metaClient.getActiveTimeline().readInstantContent(rollbackInstant,
HoodieRollbackMetadata.class);
+ List<StoragePathInfo> logFiles = listAllLogFilesInPath(hoodieTable);
+ if (tableVersion < HoodieTableVersion.EIGHT.versionCode()) {
+ // Ensure rollback metadata contains rollback log files for table
version 6
+ assertTrue(rollbackMetadata.getPartitionMetadata().values().stream()
+ .noneMatch(rollbackPartitionMetadata ->
rollbackPartitionMetadata.getRollbackLogFiles().isEmpty()));
+ // Total 3 partitions would contain 6 log files - one rollback log
file and one log file with data block
+ assertEquals(6, logFiles.size());
+ } else {
+ // Ensure rollback metadata does not contain rollback log files for
table version 8
+ assertFalse(rollbackMetadata.getPartitionMetadata().values().stream()
+ .noneMatch(rollbackPartitionMetadata ->
rollbackPartitionMetadata.getRollbackLogFiles().isEmpty()));
Review Comment:
Also the same logic might already be covered by other unit tests, so there
is no need to validate that in the functional tests?
--
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]
