fix(lock): unify default FS lock path and make CLI lock config take effect - #19794
fix(lock): unify default FS lock path and make CLI lock config take effect#19794Joy-2000 wants to merge 1 commit into
Conversation
…ffect Two related defects in the metadata-table auto-lock path: 1. Lock-path inconsistency: FileSystemBasedLockProvider.getLockConfig used the .aux folder as the default lock path, while the constructor fell back to the .hoodie meta folder. The two defaults could diverge, silently placing the lock file at different paths and breaking mutual exclusion across engines/tasks. Unify both on the table metadata path (.hoodie) by reusing a single defaultLockPath() helper. 2. Lock config never applied: RunClustering/RunCompaction appended lock options to `confs` AFTER the write client was already built, so they had no effect. Move the auto-lock injection down into HoodieCLIUtils.createHoodieWriteClient, applied after the final parameters are merged and before the client is built, guarded on the fully-merged params so any explicitly-configured lock provider is respected. This also extends the coverage from just clustering/compaction to all write procedures that go through createHoodieWriteClient. Remove the now-dead lock-injection blocks and unused imports from the two procedures. Add/extend tests covering the unified path and getLockOptions.
c600fdd to
4ddd55c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19794 +/- ##
============================================
+ Coverage 78.14% 78.23% +0.08%
- Complexity 33692 33787 +95
============================================
Files 2540 2541 +1
Lines 141413 141570 +157
Branches 17123 17180 +57
============================================
+ Hits 110513 110760 +247
+ Misses 23200 23091 -109
- Partials 7700 7719 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| */ | ||
| private static String defaultLockPath(String tablePath) { | ||
| return tablePath + StoragePath.SEPARATOR + AUXILIARYFOLDER_NAME; | ||
| return tablePath + StoragePath.SEPARATOR + HoodieTableMetaClient.METAFOLDER_NAME; |
There was a problem hiding this comment.
let's use AUXILIARYFOLDER_NAME instead.
| // 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 = |
There was a problem hiding this comment.
do we need lock for all the procedures, if not, keeping it in separate procedure looks fine.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR fixes two real defects: the divergent default FS-lock path between getLockConfig() (.aux) and the constructor fallback (.hoodie), and the CLI procedures appending lock options to confs after the write client was already built (so they were silently dropped). Unifying the path and moving the auto-lock config into createHoodieWriteClient before client construction both look correct. One cross-engine/upgrade consideration is worth double-checking in the inline comment. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One verbose inline comment in HoodieCLIUtils.scala that could be trimmed to just its non-obvious constraint; everything else looks clean.
| */ | ||
| private static String defaultLockPath(String tablePath) { | ||
| return tablePath + StoragePath.SEPARATOR + AUXILIARYFOLDER_NAME; | ||
| return tablePath + StoragePath.SEPARATOR + HoodieTableMetaClient.METAFOLDER_NAME; |
There was a problem hiding this comment.
🤖 Changing defaultLockPath from .aux to .hoodie also moves the lock location for the other getLockConfig() callers — hudi-flink (StreamerUtil.getLockConfig) and hudi-utilities (UtilHelpers), which previously resolved to .aux. During a rolling upgrade, an old-version writer holding .aux/lock and a new-version writer holding .hoodie/lock would no longer mutually exclude and could update the MDT concurrently. Have you considered the mixed-version window here, and is a migration note warranted? @nsivabalan could you weigh in on whether this cross-engine lock-path move needs a compatibility step?
| (catalogProps ++ | ||
| metaClient.getTableConfig.getProps.asScala.toMap ++ | ||
| filterHoodieConfigs(sparkSession.sqlContext.getAllConfs) ++ | ||
| conf).toMap |
There was a problem hiding this comment.
🤖 nit: the 5-line comment mostly restates what the surrounding code already shows — could you trim it to just the non-obvious constraint, something like // Must be applied before building the client; see getLockOptions for why FS atomic-creation support is required.?
5 participants
Describe the issue this Pull Request addresses
This PR fixes two related defects in the metadata-table auto-lock path that can cause silent lock failures for table-service writers.
1. Default lock-path inconsistency (correctness / data-safety)
FileSystemBasedLockProvider.getLockConfig(tablePath)derived the default lock path from the.hoodie/.auxfolder, while the provider's own constructor fell back to the.hoodiemeta folder when no path was configured. The two defaults could resolve to different directories for the same table, so two writers relying on different code paths would take locks on different files and never actually exclude each other — defeating the lock across engines/tasks.2. CLI lock config never took effect (correctness)
RunClusteringProcedure/RunCompactionProcedureappended the auto-derived lock options toconfsafter the write client had already been built fromconfs, so the lock configuration was silently dropped and these table services could update the metadata table without the intended DFS lock.Summary and Changelog
Table-service writers launched through the SQL procedures (compaction, clustering, and the other write procedures) now reliably acquire a filesystem-based lock on a single, consistent path under the table's metadata folder when the table has a metadata table and no lock provider is otherwise configured.
Changes:
FileSystemBasedLockProviderdefaultLockPath()helper and change the default lock path from the.hoodie/.auxfolder to the table metadata path.hoodie(METAFOLDER_NAME).defaultLockPath()so the constructor fallback andgetLockConfig()default can never diverge again.HoodieCLIUtils.createHoodieWriteClient!finalParameters.contains(LOCK_PROVIDER_CLASS_NAME)) so any explicitly-configured lock provider — at conf, table-config, or session level — is respected.getLockOptions(tablePath, scheme, params)accordingly.createHoodieWriteClient.RunClusteringProcedure/RunCompactionProcedure: remove the now-dead post-build lock-injection blocks and unused imports.TestFileSystemBasedLockProvider(verify the explicitgetLockConfigpath and the constructorBASE_PATHfallback resolve to the same.hoodie/lockfile) and add cases toTestHoodieCLIUtilsforgetLockOptions(supported/null scheme → FS lock config under.hoodie, unsupported scheme → empty, customhoodie.fs.atomic_creation.support→ FS lock config).Impact
.hoodie(previously the config was dropped, or could point at.aux). Users who already set a lock provider at any layer are unaffected — their provider is preserved.getLockOptionsis an internal utility whose signature changed.Risk Level
low
HoodieWriteConfig.AUTO_ADJUST_LOCK_CONFIGSdefaults tofalse, so the injected FS lock provider is not downgraded to in-process.TestFileSystemBasedLockProvider(17/17 pass) andTestHoodieCLIUtils(13/13 pass), including the new path-consistency andgetLockOptionscases.Documentation Update
Contributor's checklist