yihua commented on code in PR #13229:
URL: https://github.com/apache/hudi/pull/13229#discussion_r2078619732
##########
hudi-integ-test/src/main/java/org/apache/hudi/integ/testsuite/HoodieInlineTestSuiteWriter.java:
##########
@@ -233,7 +233,7 @@ public void commitCompaction(JavaRDD<WriteStatus> records,
JavaRDD<DeltaWriteSta
}
HoodieSparkTable<HoodieRecordPayload> table =
HoodieSparkTable.create(writeClient.getConfig(),
writeClient.getEngineContext());
HoodieCommitMetadata metadata =
CompactHelpers.getInstance().createCompactionMetadata(table, instantTime.get(),
HoodieJavaRDD.of(records), writeClient.getConfig().getSchema());
- writeClient.commitCompaction(instantTime.get(), metadata,
Option.of(extraMetadata));
+ writeClient.completeCompaction(metadata, table, instantTime.get());
Review Comment:
Have you checked L172-L173 which does not commit compaction now?
```
HoodieWriteMetadata<JavaRDD<WriteStatus>> compactionMetadata =
writeClient.compact(instantTime.get());
return compactionMetadata.getWriteStatuses();
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1510,7 +1514,7 @@ protected HoodieData<HoodieRecord>
prepRecords(Map<String, HoodieData<HoodieReco
public void performTableServices(Option<String> inFlightInstantTimestamp,
boolean requiresTimelineRefresh) {
HoodieTimer metadataTableServicesTimer = HoodieTimer.start();
boolean allTableServicesExecutedSuccessfullyOrSkipped = true;
- BaseHoodieWriteClient<?, I, ?, ?> writeClient = getWriteClient();
+ BaseHoodieWriteClient<?, I, ?, O> writeClient = getWriteClient();
Review Comment:
Any reason changing this from `?` to `O`?
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/functional/TestHoodieJavaClientOnMergeOnReadStorage.java:
##########
@@ -113,6 +114,9 @@ public void testCompactionOnMORTable() throws Exception {
Option<String> timeStamp = client.scheduleCompaction(Option.empty());
assertTrue(timeStamp.isPresent());
client.compact(timeStamp.get());
+ HoodieWriteMetadata writeMetadata = client.compact(timeStamp.get());
Review Comment:
`client.compact(timeStamp.get());` is already invoked in the line before.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1092,28 +1092,20 @@ public HoodieWriteMetadata<O> compact(String
compactionInstantTime) {
if (shouldDelegateToTableServiceManager(config, ActionType.compaction)) {
throw new UnsupportedOperationException("Compaction should be delegated
to table service manager instead of direct run.");
}
- return compact(compactionInstantTime, config.shouldAutoCommit());
- }
-
- /**
- * Commit a compaction operation. Allow passing additional meta-data to be
stored in commit instant file.
- *
- * @param compactionInstantTime Compaction Instant Time
- * @param metadata All the metadata that gets stored along with a commit
- * @param extraMetadata Extra Metadata to be stored
- */
- public void commitCompaction(String compactionInstantTime,
HoodieCommitMetadata metadata,
- Option<Map<String, String>> extraMetadata) {
- tableServiceClient.commitCompaction(compactionInstantTime, metadata,
extraMetadata);
+ return compact(compactionInstantTime, false);
Review Comment:
Other callers that need revisit in tests:
- `ITTestSchemaEvolution`:
```
private void changeTableSchema(TableOptions tableOptions, boolean
shouldCompactBeforeSchemaChanges) throws IOException {
try (HoodieFlinkWriteClient<?> writeClient =
FlinkWriteClients.createWriteClient(tableOptions.toConfig())) {
if (shouldCompactBeforeSchemaChanges) {
Option<String> compactionInstant =
writeClient.scheduleCompaction(Option.empty());
writeClient.compact(compactionInstant.get());
}
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1092,28 +1092,20 @@ public HoodieWriteMetadata<O> compact(String
compactionInstantTime) {
if (shouldDelegateToTableServiceManager(config, ActionType.compaction)) {
throw new UnsupportedOperationException("Compaction should be delegated
to table service manager instead of direct run.");
}
- return compact(compactionInstantTime, config.shouldAutoCommit());
- }
-
- /**
- * Commit a compaction operation. Allow passing additional meta-data to be
stored in commit instant file.
- *
- * @param compactionInstantTime Compaction Instant Time
- * @param metadata All the metadata that gets stored along with a commit
- * @param extraMetadata Extra Metadata to be stored
- */
- public void commitCompaction(String compactionInstantTime,
HoodieCommitMetadata metadata,
- Option<Map<String, String>> extraMetadata) {
- tableServiceClient.commitCompaction(compactionInstantTime, metadata,
extraMetadata);
+ return compact(compactionInstantTime, false);
Review Comment:
Check `HoodieMorCompactionJob` class too which calls `#compact` that does
not commit compaction after this change
```
try {
val instant = client.scheduleCompaction(Option.empty())
client.compact(instant.get())
client.clean()
} catch {
case e: Exception => System.err.println(s"Compaction failed due to", e)
} finally {
client.close()
spark.stop()
}
```
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/functional/TestHoodieJavaClientOnMergeOnReadStorage.java:
##########
@@ -113,6 +114,9 @@ public void testCompactionOnMORTable() throws Exception {
Option<String> timeStamp = client.scheduleCompaction(Option.empty());
assertTrue(timeStamp.isPresent());
client.compact(timeStamp.get());
+ HoodieWriteMetadata writeMetadata = client.compact(timeStamp.get());
+ client.commitCompaction(timeStamp.get(), writeMetadata, Option.empty());
+
Review Comment:
nit: remove empty line
--
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]
