hudi-agent commented on code in PR #19794:
URL: https://github.com/apache/hudi/pull/19794#discussion_r3954048815
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -115,8 +114,8 @@ public FileSystemBasedLockProvider(final LockConfiguration
lockConfiguration, fi
this.lockConfiguration = lockConfiguration;
String lockDirectory =
lockConfiguration.getConfig().getString(FILESYSTEM_LOCK_PATH_PROP_KEY, null);
if (StringUtils.isNullOrEmpty(lockDirectory)) {
- lockDirectory =
lockConfiguration.getConfig().getString(HoodieWriteConfig.BASE_PATH.key())
- + StoragePath.SEPARATOR + HoodieTableMetaClient.METAFOLDER_NAME;
+ // Match getLockConfig() so writers using either default share the same
lock.
+ lockDirectory =
defaultLockPath(lockConfiguration.getConfig().getString(HoodieWriteConfig.BASE_PATH.key()));
Review Comment:
🤖 This flips the default lock file for every existing writer that sets only
`hoodie.write.lock.provider=FileSystemBasedLockProvider` (no explicit
`filesystem.path`) from `<base>/.hoodie/lock` to `<base>/.hoodie/.aux/lock`.
During a rolling upgrade, an old-jar writer and a new-jar writer would take two
different lock files and no longer exclude each other — concurrent commits on
the same table. Could we call this out in release notes, or keep old-path
compatibility (e.g. honor `.hoodie/lock` if it exists)? @nsivabalan could you
weigh in on whether relocating the default FS lock path is safe for
mixed-version multi-writer setups?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieCLIUtils.scala:
##########
@@ -166,9 +172,19 @@ object HoodieCLIUtils extends Logging {
key -> value
}
- def getLockOptions(tablePath: String, schema: String, lockConfig:
TypedProperties): Map[String, String] = {
- val customSupportedFSs =
lockConfig.getStringList(HoodieCommonConfig.HOODIE_FS_ATOMIC_CREATION_SUPPORT.key,
",", new ArrayList[String])
- if (schema == null || customSupportedFSs.contains(schema) ||
StorageSchemes.isAtomicCreationSupported(schema)) {
+ /**
+ * Builds the filesystem-based lock configuration for the metadata table, or
an empty map when the
+ * table's filesystem cannot support atomic file creation (a hard
requirement for the FS lock).
+ *
Review Comment:
🤖 nit: the merge order flipped from `confs ++ getLockOptions(...)` (old
callers) to `getLockOptions(...) ++ confs` (new callers), which changes
precedence. Might be worth a line in the javadoc here noting that
`params`/caller-supplied confs are expected to win over the derived lock
options, since that's not obvious from the signature alone.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieCLIUtils.scala:
##########
@@ -60,13 +59,26 @@ object HoodieCLIUtils extends Logging {
}
// Priority: defaults < catalog props < table config < sparkSession conf <
specified conf
- val finalParameters = HoodieWriterUtils.parametersWithWriteDefaults(
+ val parameters = HoodieWriterUtils.parametersWithWriteDefaults(
(catalogProps ++
metaClient.getTableConfig.getProps.asScala.toMap ++
filterHoodieConfigs(sparkSession.sqlContext.getAllConfs) ++
conf).toMap
)
+ // Auto-config a DFS-based lock for the metadata table when the table has
an MDT and no lock
+ // provider is configured at any layer. Table-service writers created here
(compaction,
+ // clustering, clean, TTL, restore, rollback, savepoint, ...) can update
the metadata table, so
+ // they must be mutually exclusive with concurrent writers on the shared
lock path. This must be
+ // applied before building the client so the lock config actually takes
effect.
+ val finalParameters =
Review Comment:
🤖 One more that looks like the same shape: `run_ttl` (`RunTTLProcedure`)
already threads a `confs` map into `HoodieCLIUtils.createHoodieWriteClient` and
then issues a replace-commit via `startDeletePartitionCommit`/`commit`, so it
mutates the MDT with exactly the same exposure as clean/clustering — that one
should be a one-line addition.
The rollback/restore family (`RollbackToInstantTimeProcedure`,
`RestoreToInstantProcedure`, `RunRollbackInflightTableServiceProcedure`) also
writes MDT, but they pass `Map.empty` today, so they'd need a `confs`
plumb-through first — probably fine to leave to a follow-up if you want to keep
this PR scoped.
--
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]
