danny0405 commented on issue #19587: URL: https://github.com/apache/hudi/issues/19587#issuecomment-5300583751
Thanks for writing this up. I traced the three proposed boundaries through the current code and have a few comments/questions. > **1. Table-service scheduling** Moving plan generation outside the lock looks useful, but I don't think serializing only instant creation + plan-file creation is sufficient. Both clustering and compaction currently build a plan from a timeline/filesystem-view snapshot and then publish it as one operation ([clustering](https://github.com/apache/hudi/blob/master/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/cluster/ClusteringPlanActionExecutor.java#L58-L102), [compaction](https://github.com/apache/hudi/blob/master/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/compact/ScheduleCompactionActionExecutor.java#L106-L134)). If schedulers A and B build concurrently before either publishes, both can select the same file groups; serializing the two publishes does not make B's already-built plan fresh. A two-phase design therefore seems to need a validation/fencing step under the transaction lock: reload the timeline/view, verify the plan's input file groups are still eligible and do not overlap any newly pending service, and reject/rebuild on failure. A concurrency test where two planners pause after selecting the same file groups, then publish in sequence, would be valuable. > **2. Request-time generation in `startCommit`** Atomic creation of only the requested-instant filename does not cover every collision. Timeline layout maps `compaction -> commit`, `logcompaction -> deltacommit`, and `clustering -> replacecommit` ([mapping](https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/InstantComparatorV2.java#L51-L56)); latest-state filtering groups by `(requestedTime, comparableAction)` ([grouping](https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimelineLayout.java#L111-L121)). Thus two different action filenames can both be created successfully at the same requested time and later be treated as the same logical action family. Any lock-free allocation/retry should check/reserve the request time across the relevant comparable-action family, not just retry when the exact target path already exists. This is especially important because `createNewInstantTime(false)` only has the process-local monotonic guard while skipping the distributed time-generator lock. > **3. `finalizeWrite`** This seems separable if the order is explicitly: finalize first; then acquire the transaction lock; then reload current table/timeline state and perform conflict resolution, Metadata Table update, and timeline completion in one critical section. It should not be split as conflict-resolution-under-lock → unlock → finalize → relock, because a commit completing in that gap invalidates the conflict check. Also, `finalizeWrite` is not read-only: marker reconciliation deletes invalid data files ([implementation](https://github.com/apache/hudi/blob/master/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java#L747-L774)). Before moving it, I would add interleaving tests against rollback/failed-write cleaning for the same instant (including both marker implementations and an expired heartbeat) to prove those paths cannot delete a file selected as valid while finalize is outside the transaction. > **4. Metrics** There are already two lock timers behind `hoodie.metrics.lock.enable`: `lock.request.latency` and `lock.acquire.duration` ([metrics](https://github.com/apache/hudi/blob/master/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/HoodieLockMetrics.java#L38-L43), [config](https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/config/metrics/HoodieMetricsConfig.java#L99-L110)). The latter is updated on unlock and is effectively hold time. One gap is that `lock.request.latency` is started inside each retry attempt ([LockManager](https://github.com/apache/hudi/blob/master/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/LockManager.java#L76-L90)), so it does not capture the total caller-visible wait across retry backoff. An end-to-end timer around `TransactionManager.beginStateChange`, ideally tagged by operation/action, would make the reported Flink stalls much easier to attribute. -- 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]
