danny0405 commented on a change in pull request #4420:
URL: https://github.com/apache/hudi/pull/4420#discussion_r797294083
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -464,27 +464,43 @@ protected void postCommit(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata me
}
protected void runTableServicesInline(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata metadata, Option<Map<String, String>> extraMetadata) {
- if (config.areAnyTableServicesInline()) {
+ if (config.areAnyTableServicesInline() ||
config.scheduleInlineTableServices()) {
if (config.isMetadataTableEnabled()) {
Review comment:
Do you think we should returns true for only scheduling(non-execute)
table service in `areAnyTableServicesInline`, personally i think there are
responsibilities similarity for these two methods here and they are confusing.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -464,27 +464,43 @@ protected void postCommit(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata me
}
protected void runTableServicesInline(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata metadata, Option<Map<String, String>> extraMetadata) {
- if (config.areAnyTableServicesInline()) {
+ if (config.areAnyTableServicesInline() ||
config.scheduleInlineTableServices()) {
if (config.isMetadataTableEnabled()) {
table.getHoodieView().sync();
}
// Do an inline compaction if enabled
if (config.inlineCompactionEnabled()) {
runAnyPendingCompactions(table);
metadata.addMetadata(HoodieCompactionConfig.INLINE_COMPACT.key(),
"true");
- inlineCompact(extraMetadata);
+ inlineScheduleCompactAndOptionallyExecute(extraMetadata,
!config.scheduleInlineCompaction());
} else {
metadata.addMetadata(HoodieCompactionConfig.INLINE_COMPACT.key(),
"false");
}
+ // if just inline schedule is enabled
+ if (!config.inlineCompactionEnabled() &&
config.scheduleInlineCompaction()
+ &&
!table.getActiveTimeline().getWriteTimeline().filterPendingCompactionTimeline().getInstants().findAny().isPresent())
{
+ // proceed only if there are no pending compactions
Review comment:
Can we just add a tool method in `HoodieActiveTimeline` for decision
`table.getActiveTimeline().getWriteTimeline().filterPendingCompactionTimeline().getInstants().findAny().isPresent()`
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieClusteringConfig.java
##########
@@ -177,6 +178,16 @@
.withDocumentation("Determines how to handle updates, deletes to file
groups that are under clustering."
+ " Default strategy just rejects the update");
+ public static final ConfigProperty<String> SCHEDULE_INLINE_CLUSTERING =
ConfigProperty
+ .key("hoodie.clustering.schedule.inline")
Review comment:
`SCHEDULE_INLINE_CLUSTERING` -> `CLUSTERING_SCHEDULE_INLINE`
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -464,27 +464,43 @@ protected void postCommit(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata me
}
protected void runTableServicesInline(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata metadata, Option<Map<String, String>> extraMetadata) {
- if (config.areAnyTableServicesInline()) {
+ if (config.areAnyTableServicesInline() ||
config.scheduleInlineTableServices()) {
if (config.isMetadataTableEnabled()) {
table.getHoodieView().sync();
}
// Do an inline compaction if enabled
if (config.inlineCompactionEnabled()) {
runAnyPendingCompactions(table);
metadata.addMetadata(HoodieCompactionConfig.INLINE_COMPACT.key(),
"true");
- inlineCompact(extraMetadata);
+ inlineScheduleCompactAndOptionallyExecute(extraMetadata,
!config.scheduleInlineCompaction());
} else {
metadata.addMetadata(HoodieCompactionConfig.INLINE_COMPACT.key(),
"false");
}
+ // if just inline schedule is enabled
+ if (!config.inlineCompactionEnabled() &&
config.scheduleInlineCompaction()
+ &&
!table.getActiveTimeline().getWriteTimeline().filterPendingCompactionTimeline().getInstants().findAny().isPresent())
{
+ // proceed only if there are no pending compactions
Review comment:
Can we also add a tool method for decision
`config.inlineCompactionEnabled() && config.scheduleInlineCompaction()` in
config.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -1005,13 +1021,16 @@ protected void rollbackFailedWrites(Map<String,
Option<HoodiePendingRollbackInfo
/**
* Performs a compaction operation on a table, serially before or after an
insert/upsert action.
+ * Scheduling is done inline, and optionally execution as well.
*/
- protected Option<String> inlineCompact(Option<Map<String, String>>
extraMetadata) {
+ protected Option<String>
inlineScheduleCompactAndOptionallyExecute(Option<Map<String, String>>
extraMetadata, boolean executeInline) {
Option<String> compactionInstantTimeOpt =
scheduleCompaction(extraMetadata);
Review comment:
Personally i prefer to keep the old method and add a new one called
`inlineScheduleCompact` which is only for scheduling purposes.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -464,27 +464,43 @@ protected void postCommit(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata me
}
protected void runTableServicesInline(HoodieTable<T, I, K, O> table,
HoodieCommitMetadata metadata, Option<Map<String, String>> extraMetadata) {
- if (config.areAnyTableServicesInline()) {
+ if (config.areAnyTableServicesInline() ||
config.scheduleInlineTableServices()) {
if (config.isMetadataTableEnabled()) {
table.getHoodieView().sync();
}
// Do an inline compaction if enabled
if (config.inlineCompactionEnabled()) {
runAnyPendingCompactions(table);
metadata.addMetadata(HoodieCompactionConfig.INLINE_COMPACT.key(),
"true");
- inlineCompact(extraMetadata);
+ inlineScheduleCompactAndOptionallyExecute(extraMetadata,
!config.scheduleInlineCompaction());
Review comment:
The decision already indicate deterministic incline `schedule and
execute` table services here, so can we just keep the existing method
`inlineCompact` here and made a new method for only scheduling purpose.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
##########
@@ -90,6 +90,12 @@
.withDocumentation("When set to true, compaction service is triggered
after each write. While being "
+ " simpler operationally, this adds extra latency on the write
path.");
+ public static final ConfigProperty<String> SCHEDULE_ASYNC_COMPACT =
ConfigProperty
+ .key("hoodie.compact.schedule.async")
+ .defaultValue("false")
Review comment:
`COMPACT_SCHEDULE_INLINE` please to keep inline with the string options
sequence.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -1116,13 +1135,16 @@ protected boolean scheduleCleaningAtInstant(String
instantTime, Option<Map<Strin
/**
* Executes a clustering plan on a table, serially before or after an
insert/upsert action.
+ * Schedules clustering inline and may be optionally execute.
*/
- protected Option<String> inlineCluster(Option<Map<String, String>>
extraMetadata) {
+ protected Option<String>
inlineScheduleClusterAndOptionallyExecute(Option<Map<String, String>>
extraMetadata, boolean executeInline) {
Option<String> clusteringInstantOpt = scheduleClustering(extraMetadata);
- clusteringInstantOpt.ifPresent(clusteringInstant -> {
- // inline cluster should auto commit as the user is never given control
- cluster(clusteringInstant, true);
- });
+ if (executeInline) {
+ clusteringInstantOpt.ifPresent(clusteringInstant -> {
Review comment:
ditto
--
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]
