nsivabalan commented on PR #19205: URL: https://github.com/apache/hudi/pull/19205#issuecomment-5272728187
@danny0405 @voonhous @cshuo — this is ready for another look when you have time. Since the last round the design changed in one substantive way, so I would not rely on your earlier reading of the validation logic. ### What changed **The write-path rule is now plain equality of resolved modes.** `resolveMetaFieldsModeForWrite` is gone; nothing reconciles the write config against the table any more. `validateAgainstTableProperties` compares `writeConfig.getMetaFieldsMode()` against `tableConfig.getMetaFieldsMode()` and throws on any difference. That came out of @danny0405's point that inference does not belong in the validator. Removing it forced the rule to get stricter: a writer that states nothing resolves to the `ALL` default, so it now fails against any table that is not `ALL`. An `ALL` table — the default — still accepts a writer that says nothing, which is why existing callers and table services are unaffected. The reason this is worth the strictness rather than inheriting: the mode is read again below `initTable`, in the handles and the Avro/Spark writer factories, and three of the eight production `getFileWriter` call sites hold no table config at all. Inference at `initTable` would fix that one method and leave everything downstream reading a write config that disagrees with the table. **Consequence for @cshuo's restart case:** it is now a hard failure rather than inheritance, which reverses what I told you on that thread. Details and the migration path are in my reply there. **Downgrade** follows the three-way rule agreed offline: `ALL`/`NONE` drop the mode; a selective mode restated by the writer is retained; a selective mode not restated is rejected rather than silently collapsed to `NONE`. ### Verification CI is green on 16 of 17 jobs. The one red is `test-spark-java17-java-tests-part2`, which I am still checking — my fixes cleared all 8 meta-fields failures in that job (8 → 0), and what remains is 40 timestamp assertions in `TestHoodieClientOnCopyOnWriteStorage` clustering tests. Current evidence says they are not from this PR: the stack touches no clustering or preserve-metadata files, the assertion and its test setup are unchanged from `master`, and the assertion sits inside `if (config.populateMetaFields())` — the `ALL` path, which this PR leaves alone. Deltas are 700–2400 ms, so timing-sensitivity is the likely explanation. I have re-run the job to see whether it reproduces. If any of you recognise it as a known flake, that would save me a round. Six test fixtures needed updating along the way, all the same shape: the fixture created a table with meta fields populated and then built a writer asking for `NONE`, which the old code let through silently. Each is now stated on the table rather than only on the writer. ### Threads Every open thread has a reply. Three are push-backs where I disagreed and gave evidence rather than just changing the code — the merge-handle `_hoodie_file_name` clearing, `setDefaults`, and the `HoodieSparkRecord` two-property read. Happy to be overruled on any of them; I would rather that than have them merged unexamined. The PR description has been rewritten to match what actually shipped, including the two user-visible behavior changes and the follow-ups (#19206, #19378, #19568). -- 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]
