yihua commented on code in PR #18295:
URL: https://github.com/apache/hudi/pull/18295#discussion_r3055465899
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieLockConfig.java:
##########
@@ -249,6 +266,65 @@ public static HoodieLockConfig.Builder newBuilder() {
return new HoodieLockConfig.Builder();
}
+ /**
+ * Build a {@link HoodieLockConfig} by copying lock-related configs from the
given write config
+ * based on the configured lock provider. Only built-in lock providers are
supported.
+ *
+ * @param lockProviderClass the lock provider class name
+ * @param writeConfig the write config to copy lock properties from
+ * @return a new HoodieLockConfig with the relevant lock properties
+ * @throws HoodieException if the lock provider is not a built-in provider
+ */
+ public static HoodieLockConfig getLockConfigForBuiltInLockProvider(String
lockProviderClass, HoodieWriteConfig writeConfig) {
+ TypedProperties dataProps = writeConfig.getProps();
+ Properties lockProps = new Properties();
+ lockProps.put(LOCK_PROVIDER_CLASS_NAME.key(), lockProviderClass);
+
+ // Common lock configs used by all providers
+ lockProps.put(LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS.key(),
+
writeConfig.getStringOrDefault(LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS));
+ lockProps.put(LOCK_ACQUIRE_RETRY_MAX_WAIT_TIME_IN_MILLIS.key(),
+
writeConfig.getStringOrDefault(LOCK_ACQUIRE_RETRY_MAX_WAIT_TIME_IN_MILLIS));
+ lockProps.put(LOCK_ACQUIRE_CLIENT_RETRY_WAIT_TIME_IN_MILLIS.key(),
+
writeConfig.getStringOrDefault(LOCK_ACQUIRE_CLIENT_RETRY_WAIT_TIME_IN_MILLIS));
+ lockProps.put(LOCK_ACQUIRE_NUM_RETRIES.key(),
+ writeConfig.getStringOrDefault(LOCK_ACQUIRE_NUM_RETRIES));
+ lockProps.put(LOCK_ACQUIRE_CLIENT_NUM_RETRIES.key(),
+ writeConfig.getStringOrDefault(LOCK_ACQUIRE_CLIENT_NUM_RETRIES));
+ lockProps.put(LOCK_ACQUIRE_WAIT_TIMEOUT_MS.key(),
+
String.valueOf(writeConfig.getIntOrDefault(LOCK_ACQUIRE_WAIT_TIMEOUT_MS)));
+ lockProps.put(LOCK_HEARTBEAT_INTERVAL_MS.key(),
+
String.valueOf(writeConfig.getIntOrDefault(LOCK_HEARTBEAT_INTERVAL_MS)));
+
+ // Provider-specific configs
+ if
(FileSystemBasedLockProvider.class.getCanonicalName().equals(lockProviderClass))
{
+ copyPropsWithPrefix(dataProps, lockProps,
LockConfiguration.FILESYSTEM_BASED_LOCK_PROPERTY_PREFIX);
+ } else if
(ZookeeperBasedLockProvider.class.getCanonicalName().equals(lockProviderClass)
+ ||
ZookeeperBasedImplicitBasePathLockProvider.class.getCanonicalName().equals(lockProviderClass))
{
+ copyPropsWithPrefix(dataProps, lockProps,
LockConfiguration.ZOOKEEPER_BASED_LOCK_PROPERTY_PREFIX);
+ } else if
(HIVE_METASTORE_BASED_LOCK_PROVIDER_CLASS.equals(lockProviderClass)) {
+ copyPropsWithPrefix(dataProps, lockProps,
LockConfiguration.HIVE_METASTORE_LOCK_PROPERTY_PREFIX);
+ } else if (DYNAMODB_BASED_LOCK_PROVIDER_CLASS.equals(lockProviderClass)
+ ||
DYNAMODB_BASED_IMPLICIT_PARTITION_KEY_LOCK_PROVIDER_CLASS.equals(lockProviderClass))
{
+ copyPropsWithPrefix(dataProps, lockProps,
DYNAMODB_BASED_LOCK_PROPERTY_PREFIX);
+ } else if
(StorageBasedLockProvider.class.getCanonicalName().equals(lockProviderClass)) {
+ copyPropsWithPrefix(dataProps, lockProps,
STORAGE_BASED_LOCK_PROPERTY_PREFIX);
+ } else {
+ throw new HoodieException(writeConfig.getWriteConcurrencyMode()
+ + " is only supported for built-in lock providers. Unsupported lock
provider: " + lockProviderClass);
+ }
+
+ return newBuilder().fromProperties(lockProps).build();
Review Comment:
_⚠️ Potential issue_ | _🟠 Major_
**Preserve inferred provider inputs when rebuilding the lock config.**
This helper creates a fresh `HoodieLockConfig`, but it only carries
lock-prefixed keys. `ZK_LOCK_KEY` still infers from
`HoodieWriteConfig.TBL_NAME` on Line 203, so a setup that relies on the default
ZK lock key will lose it here and the derived built-in lock config becomes
incomplete.
<details>
<summary>Minimal fix sketch</summary>
```diff
public static HoodieLockConfig getLockConfigForBuiltInLockProvider(String
lockProviderClass, HoodieWriteConfig writeConfig) {
TypedProperties dataProps = writeConfig.getProps();
Properties lockProps = new Properties();
lockProps.put(LOCK_PROVIDER_CLASS_NAME.key(), lockProviderClass);
+ if (writeConfig.contains(HoodieWriteConfig.TBL_NAME)) {
+ lockProps.put(HoodieWriteConfig.TBL_NAME.key(),
+ writeConfig.getString(HoodieWriteConfig.TBL_NAME));
+ }
```
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately
replaces the highlighted code, contains no missing lines, and has no issues
with indentation. Thoroughly test & benchmark the code to ensure it meets the
requirements.
```suggestion
public static HoodieLockConfig getLockConfigForBuiltInLockProvider(String
lockProviderClass, HoodieWriteConfig writeConfig) {
TypedProperties dataProps = writeConfig.getProps();
Properties lockProps = new Properties();
lockProps.put(LOCK_PROVIDER_CLASS_NAME.key(), lockProviderClass);
if (writeConfig.contains(HoodieWriteConfig.TBL_NAME)) {
lockProps.put(HoodieWriteConfig.TBL_NAME.key(),
writeConfig.getString(HoodieWriteConfig.TBL_NAME));
}
// Common lock configs used by all providers
lockProps.put(LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS.key(),
writeConfig.getStringOrDefault(LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS));
lockProps.put(LOCK_ACQUIRE_RETRY_MAX_WAIT_TIME_IN_MILLIS.key(),
writeConfig.getStringOrDefault(LOCK_ACQUIRE_RETRY_MAX_WAIT_TIME_IN_MILLIS));
lockProps.put(LOCK_ACQUIRE_CLIENT_RETRY_WAIT_TIME_IN_MILLIS.key(),
writeConfig.getStringOrDefault(LOCK_ACQUIRE_CLIENT_RETRY_WAIT_TIME_IN_MILLIS));
lockProps.put(LOCK_ACQUIRE_NUM_RETRIES.key(),
writeConfig.getStringOrDefault(LOCK_ACQUIRE_NUM_RETRIES));
lockProps.put(LOCK_ACQUIRE_CLIENT_NUM_RETRIES.key(),
writeConfig.getStringOrDefault(LOCK_ACQUIRE_CLIENT_NUM_RETRIES));
lockProps.put(LOCK_ACQUIRE_WAIT_TIMEOUT_MS.key(),
String.valueOf(writeConfig.getIntOrDefault(LOCK_ACQUIRE_WAIT_TIMEOUT_MS)));
lockProps.put(LOCK_HEARTBEAT_INTERVAL_MS.key(),
String.valueOf(writeConfig.getIntOrDefault(LOCK_HEARTBEAT_INTERVAL_MS)));
// Provider-specific configs
if
(FileSystemBasedLockProvider.class.getCanonicalName().equals(lockProviderClass))
{
copyPropsWithPrefix(dataProps, lockProps,
LockConfiguration.FILESYSTEM_BASED_LOCK_PROPERTY_PREFIX);
} else if
(ZookeeperBasedLockProvider.class.getCanonicalName().equals(lockProviderClass)
||
ZookeeperBasedImplicitBasePathLockProvider.class.getCanonicalName().equals(lockProviderClass))
{
copyPropsWithPrefix(dataProps, lockProps,
LockConfiguration.ZOOKEEPER_BASED_LOCK_PROPERTY_PREFIX);
} else if
(HIVE_METASTORE_BASED_LOCK_PROVIDER_CLASS.equals(lockProviderClass)) {
copyPropsWithPrefix(dataProps, lockProps,
LockConfiguration.HIVE_METASTORE_LOCK_PROPERTY_PREFIX);
} else if (DYNAMODB_BASED_LOCK_PROVIDER_CLASS.equals(lockProviderClass)
||
DYNAMODB_BASED_IMPLICIT_PARTITION_KEY_LOCK_PROVIDER_CLASS.equals(lockProviderClass))
{
copyPropsWithPrefix(dataProps, lockProps,
DYNAMODB_BASED_LOCK_PROPERTY_PREFIX);
} else if
(StorageBasedLockProvider.class.getCanonicalName().equals(lockProviderClass)) {
copyPropsWithPrefix(dataProps, lockProps,
STORAGE_BASED_LOCK_PROPERTY_PREFIX);
} else {
throw new HoodieException(writeConfig.getWriteConcurrencyMode()
" is only supported for built-in lock providers. Unsupported lock
provider: " + lockProviderClass);
}
return newBuilder().fromProperties(lockProps).build();
```
</details>
<!-- suggestion_end -->
<details>
<summary>🤖 Prompt for AI Agents</summary>
```
Verify each finding against the current code and only fix it if needed.
In
`@hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieLockConfig.java`
around lines 278 - 317, getLockConfigForBuiltInLockProvider builds lockProps
only from lock-prefixed keys, causing inferred values like ZK_LOCK_KEY (which
defaults from HoodieWriteConfig.TBL_NAME) to be lost; fix by copying any
needed
non-prefixed inferred entries into lockProps before building the
HoodieLockConfig. Specifically, in getLockConfigForBuiltInLockProvider (use
dataProps and lockProps), after the provider-specific
copyPropsWithPrefix(...)
call but before newBuilder().fromProperties(...).build(), check for keys that
are inferred by providers (e.g., ZK_LOCK_KEY and any other provider-specific
inferred keys) and if absent in lockProps, populate them from
writeConfig.getProps()/HoodieWriteConfig.TBL_NAME or compute the same default
the provider expects so the rebuilt HoodieLockConfig preserves inferred
inputs.
```
</details>
<!--
fingerprinting:phantom:medusa:grasshopper:3ff5d5a1-02a2-40b7-8f7e-b70bc4eba263
-->
<!-- This is an auto-generated comment by CodeRabbit -->
— *CodeRabbit*
([original](https://github.com/yihua/hudi/pull/27#discussion_r3055465495))
(source:comment#3055465495)
--
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]
