ad1happy2go commented on PR #19304: URL: https://github.com/apache/hudi/pull/19304#issuecomment-5799028401
Thanks for the review — pushed fixes for the ones I could verify, and left two for you. **Done (b7ba720):** - `:451` — removed the `KeyGenUtils.isComplexKeyGeneratorWithSingleRecordKeyField` wrapper; callers now use `tableConfig.isComplexKeyGenWithSingleRecordKeyField()` directly. It had no production callers, only tests. - `:501` — renamed to `requireComplexKeyGenEncodingTracked`. - `:486` (nit) — reads the config through `ConfigUtils` now. Worth noting it needs the `defaultValue` overload: the two-arg `getStringWithAltKeys` throws when the property is absent, which is the normal case for a table that has not recorded an encoding. - `:669` / `:681` — dropped the archive listing, bounded the scan to the 20 most recent commits, and added a warning naming how many were inspected. One deliberate difference on the last point. Returning `Option.empty()` unconditionally outside the loop would make a table that was never written to "undetermined", and the caller throws on undetermined — so brand-new tables would fail to write. I kept that case explicit instead: an empty completed timeline returns `FIELD_PREFIXED` (nothing was ever written, so no stored key can disagree), everything else scans the 20 most recent and returns empty if none yields a record key. The behaviour change versus before is that a table whose commits have all been archived now reads as "never written" rather than "undetermined" — that is the case `hasArchivedCommits` was guarding, so let me know if you would rather keep a check for it. **Left for you:** - `:486` (`protected` in `BaseKeyGenerator`) — your condition holds; the only callers are `ComplexAvroKeyGenerator`, the Spark `ComplexKeyGenerator` and tests. It is a move across `hudi-client-common` and `hudi-spark-client`, so I did not want to land it unreviewed alongside the rest. Happy to do it if you want it in this PR. - `:574` (locking) — I checked the call sites rather than change the locking. The three production callers are `DataSourceUtils.createHoodieClient` (before the write client is constructed), `StreamSync` (before `new SparkRDDWriteClient`), and `HoodieJavaWriteClient` via the `initTable` hook; every `initTable` call site I traced is at the top of a public API method, not inside `beginStateChange`. So I found no double-locking path, though I would not call that exhaustive. Whether the lock can be dropped is your call. Local verification of the above: 208 tests green in `hudi-client-common` — `TestKeyGenUtils` 33, `TestComplexAvroKeyGenerator` 27, `TestBaseHoodieWriteClient` 98, `TestEightToNineUpgradeHandler` 24, `TestSevenToEightUpgradeHandler` 14, `TestNineToEightDowngradeHandler` 12; checkstyle clean. Separately, CI on `2d8c44d178` had `TestHoodieTableSource.testRecordLevelIndexPruningHonorsRecordedEncoding[2]` failing with `HoodieException: Failed to start operator coordinator` at `TestData.writeData`. Flagging in case it is not already on your radar — it predates these changes. -- 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]
