vinothchandar commented on a change in pull request #4406:
URL: https://github.com/apache/hudi/pull/4406#discussion_r773236194
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
##########
@@ -2221,11 +2236,17 @@ protected void setDefaults() {
HoodiePayloadConfig.newBuilder().fromProperties(writeConfig.getProps()).build());
writeConfig.setDefaultOnCondition(!isMetadataConfigSet,
HoodieMetadataConfig.newBuilder().withEngineType(engineType).fromProperties(writeConfig.getProps()).build());
- writeConfig.setDefaultOnCondition(!isLockConfigSet,
-
HoodieLockConfig.newBuilder().fromProperties(writeConfig.getProps()).build());
writeConfig.setDefaultOnCondition(!isPreCommitValidationConfigSet,
HoodiePreCommitValidatorConfig.newBuilder().fromProperties(writeConfig.getProps()).build());
writeConfig.setDefaultValue(TIMELINE_LAYOUT_VERSION_NUM,
String.valueOf(TimelineLayoutVersion.CURR_VERSION));
+
+ if (!isLockConfigSet) {
Review comment:
line of comment explaining why this is done.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
##########
@@ -1802,10 +1803,24 @@ public WriteConcurrencyMode getWriteConcurrencyMode() {
return WriteConcurrencyMode.fromValue(getString(WRITE_CONCURRENCY_MODE));
}
- public Boolean inlineTableServices() {
+ /**
+ * Are any table services configured to run inline?
+ *
+ * @return True if any table services are configured to run inline, false
otherwise.
+ */
+ public Boolean isAnyTableServicesInline() {
return inlineClusteringEnabled() || inlineCompactionEnabled() ||
isAutoClean();
}
+ /**
+ * Are any table services configured to run async?
+ *
+ * @return True if any table services are configured to run async, false
otherwise.
+ */
+ public Boolean isAnyTableServicesAsync() {
Review comment:
same here.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java
##########
@@ -460,7 +460,7 @@ 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.inlineTableServices()) {
+ if (config.isAnyTableServicesInline()) {
Review comment:
rename: inlineAnyTableServices() ?
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
##########
@@ -2221,11 +2236,17 @@ protected void setDefaults() {
HoodiePayloadConfig.newBuilder().fromProperties(writeConfig.getProps()).build());
writeConfig.setDefaultOnCondition(!isMetadataConfigSet,
HoodieMetadataConfig.newBuilder().withEngineType(engineType).fromProperties(writeConfig.getProps()).build());
- writeConfig.setDefaultOnCondition(!isLockConfigSet,
-
HoodieLockConfig.newBuilder().fromProperties(writeConfig.getProps()).build());
writeConfig.setDefaultOnCondition(!isPreCommitValidationConfigSet,
HoodiePreCommitValidatorConfig.newBuilder().fromProperties(writeConfig.getProps()).build());
writeConfig.setDefaultValue(TIMELINE_LAYOUT_VERSION_NUM,
String.valueOf(TimelineLayoutVersion.CURR_VERSION));
+
+ if (!isLockConfigSet) {
+ HoodieLockConfig.Builder lockConfigBuilder =
HoodieLockConfig.newBuilder().fromProperties(writeConfig.getProps());
Review comment:
HoodieWriteConfig is what we use even during table service schedule and
execution. Could we think thru a little on whether we could accidentally
disable a configured lock provider in any of those scenarios
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java
##########
@@ -460,7 +460,7 @@ 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.inlineTableServices()) {
+ if (config.isAnyTableServicesInline()) {
Review comment:
or `areAnyTableServicesInline()`
##########
File path:
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/config/TestHoodieWriteConfig.java
##########
@@ -104,6 +112,91 @@ public void testDefaultMarkersTypeAccordingToEngineType() {
EngineType.JAVA, MarkerType.DIRECT));
}
+ @Test
+ public void testDefaultLockProviderWhenAsyncServicesEnabled() {
+ final String inProcessLockProviderClassName =
InProcessLockProvider.class.getCanonicalName();
+
+ // Any async clustering enabled should use InProcess lock provider
+ // as default when no other lock provider is set.
+
+ // 1. Async clustering
+ HoodieWriteConfig writeConfig = createWriteConfig(new HashMap<String,
String>() {
+ {
+ put(ASYNC_CLUSTERING_ENABLE.key(), "true");
+ put(INLINE_COMPACT.key(), "true");
+ put(AUTO_CLEAN.key(), "true");
+ put(ASYNC_CLEAN.key(), "false");
+ }
+ });
+ assertTrue(writeConfig.isAnyTableServicesAsync());
+ assertEquals(inProcessLockProviderClassName,
writeConfig.getLockProviderClass());
+
+ // 2. Async clean
+ writeConfig = createWriteConfig(new HashMap<String, String>() {
+ {
+ put(ASYNC_CLUSTERING_ENABLE.key(), "false");
+ put(INLINE_COMPACT.key(), "true");
+ put(AUTO_CLEAN.key(), "true");
+ put(ASYNC_CLEAN.key(), "true");
+ }
+ });
+ assertTrue(writeConfig.isAnyTableServicesAsync());
+ assertEquals(inProcessLockProviderClassName,
writeConfig.getLockProviderClass());
+
+ // 3. Async compaction
+ writeConfig = createWriteConfig(new HashMap<String, String>() {
+ {
+ put(ASYNC_CLUSTERING_ENABLE.key(), "false");
+ put(INLINE_COMPACT.key(), "false");
+ put(AUTO_CLEAN.key(), "true");
+ put(ASYNC_CLEAN.key(), "false");
+ }
+ });
+ assertTrue(writeConfig.isAnyTableServicesAsync());
+ assertEquals(inProcessLockProviderClassName,
writeConfig.getLockProviderClass());
+
+ // 4. All inline services
+ writeConfig = createWriteConfig(new HashMap<String, String>() {
+ {
+ put(ASYNC_CLUSTERING_ENABLE.key(), "false");
+ put(INLINE_COMPACT.key(), "true");
+ put(AUTO_CLEAN.key(), "true");
+ put(ASYNC_CLEAN.key(), "false");
+ }
+ });
+ assertFalse(writeConfig.isAnyTableServicesAsync());
+ assertTrue(writeConfig.isAnyTableServicesInline());
+ assertEquals(HoodieLockConfig.LOCK_PROVIDER_CLASS_NAME.defaultValue(),
writeConfig.getLockProviderClass());
+
+ // 5. User override for the lock provider should always take the precedence
+ writeConfig = HoodieWriteConfig.newBuilder()
+ .withPath("/tmp")
+ .withLockConfig(HoodieLockConfig.newBuilder()
+ .withLockProvider(FileSystemBasedLockProviderTestClass.class)
+ .build())
+ .build();
+ assertEquals(FileSystemBasedLockProviderTestClass.class.getName(),
writeConfig.getLockProviderClass());
+
+ // Default config should have default lock provider
+ writeConfig = createWriteConfig(Collections.emptyMap());
+ if (!writeConfig.isAnyTableServicesAsync()) {
+ assertEquals(HoodieLockConfig.LOCK_PROVIDER_CLASS_NAME.defaultValue(),
writeConfig.getLockProviderClass());
+ } else {
+ assertEquals(inProcessLockProviderClassName,
writeConfig.getLockProviderClass());
+ }
+ }
+
+ private HoodieWriteConfig createWriteConfig(Map<String, String> configs) {
+ final Properties properties = new Properties();
+ for (Map.Entry<String, String> entry : configs.entrySet()) {
Review comment:
configs.forEach to be shorter?
--
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]
