Joy-2000 commented on PR #19791: URL: https://github.com/apache/hudi/pull/19791#issuecomment-5462570015
Thanks for the detailed review @voonhous — all points addressed. Summary below. **Engine-agnostic backstop.** You're right that the per-engine guards left the Hudi Streamer path exposed. I moved the real safety net down to `BaseHoodieWriteClient#preWrite` — the single chokepoint every overwrite entry point funnels through — keyed on `config.getWriteConcurrencyMode().isNonBlockingConcurrencyControl() && WriteOperationType.isOverwrite(writeOperationType)`. The per-engine checks (Spark `HoodieSparkSqlWriter`, Flink `OptionsResolver`) are kept only to fail fast with a friendlier message; they're no longer load-bearing. **Flink single funnel.** Consolidated the check to the top of `Pipelines#hoodieStreamWrite`, which the table sink, the Flink Streamer (`--op insert_overwrite`), and Sink V2 all pass through, and removed the duplicated checks from `HoodieTableSink` and `PipelinesV2`. This closes the `HoodieFlinkStreamer` gap you flagged. **Message literal → one validator.** Went a step further than hoisting the string: the whole rule now lives in one place — `WriteConcurrencyMode.checkInsertOverwriteSupported(isNbcc, isOverwrite)` — which owns both the message constant and the thrown type. All four sites route through it: `preWrite`, `HoodieSparkSqlWriter`, `OptionsResolver`, and the pre-existing `BaseSparkBucketIndexBucketInfoGetter` partitioner check. This also **unifies the exception type to `HoodieException`** everywhere — the Flink `OptionsResolver` and the partitioner previously threw `IllegalArgumentException` via `ValidationUtils.checkArgument`, which was the inconsistency you flagged. (Tests match on message, and the Flink IT / `ITTestHoodieDataSource` use `findThrowableWithMessage`, so they stay green; I updated the one `TestOptionsResolver` assertion that pinned `IllegalArgumentException`.) The call sites themselves can't be collapsed — `bulkInsertAsRow` calls `writeClient.startCommit` directl y and bypasses `preWrite`, so each entry point still needs its own guard — but they now share a single source of truth for the rule. **Enum + Locale.** Added the restriction clause to the `@EnumFieldDescription` on `NON_BLOCKING_CONCURRENCY_CONTROL`, and fixed `isNonBlockingConcurrencyControl(String)` to use `Locale.ROOT` like its sibling. **Case sensitivity.** Both `sparkSqlInsertIntoOperation` and `deduceOverwriteConfig` now lower-case the operation once with `Locale.ROOT` (the `deduceOverwriteConfig` one is the dangerous path you noted — uppercase could otherwise delete the table path). **scaladoc / ExceptionUtils.** Restored the `resolvePartitionWildcards` summary line, and the Flink IT now uses `ExceptionUtils.findThrowableWithMessage(...)` instead of the hand-rolled walker. **Tests** (moved to their conventional home in `TestInsertTable3`): - Positive routing: added an uppercase leg to `Test Insert Overwrite Bucket Index Table`, so `BULK_INSERT` still replaces rows instead of silently appending. - NBCC rejection: one consolidated test that explicitly pins `hoodie.datasource.write.operation` to each overwrite variant (`insert_overwrite` / `insert_overwrite_table`) × row-writer bulk_insert off/on, and asserts the **active timeline is empty** after failure — red against the old late partitioner check, proving we now fail before `startCommit`. - Dropped the redundant `ITTestDataStreamV2Write` mini-cluster test (the check moved to `hoodieStreamWrite`, and `TestOptionsResolver` already pins it). - Guard precision: added `TestWriteConcurrencyMode` in hudi-common — a direct unit test of the shared `checkInsertOverwriteSupported` validator, asserting both overwrite variants are rejected under NB-CC and every non-overwrite operation (e.g. `insert into` / upsert) still passes, so the guard can't over-block appends. **Description.** Updated to state that the Spark RDD path already rejected this since HUDI-8866 (`BaseSparkBucketIndexBucketInfoGetter`), and that this PR adds (a) failing *before* `initTable`/`startCommit`, and (b) covering the row-writer bulk_insert overwrite path — so the partitioner check isn't read as missed. **On the broader invariant.** Fully agree with you: the real invariant is *a fixed-id file group must never be listed in a replacecommit*, and `delete_partition` + re-insert hits the same trap. I've called this out in the description. I intentionally scoped **this** PR to `insert overwrite` as a narrow fail-early guard — `isOverwrite(...)` deliberately excludes `DELETE_PARTITION`, since a standalone `delete_partition` under NB-CC is valid and loses no data, so blanket-rejecting it would be a regression. The proper fix (bumping the `-0` generation in `newBucketFileIdForNBCC` on replace, so re-inserts land on a fresh id) is a larger change with its own compatibility surface — I'd prefer to track it as a separate issue follow-up rather than stack more per-operation guards. Also noting your FYI on the `row.writer.enable=false` silent-drop path (`BULKINSERT_OVERWRITE_OPERATION_TYPE` having no reader) — that's pre-existing and orthogonal to NB-CC (it produces no replacecommit, so no data-loss via the replaced-id mechanism); -- 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]
