nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767516450
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java:
##########
@@ -478,6 +478,8 @@ HoodieTableMetaClient
initializeEmptyTable(HoodieTableMetaClient.TableBuilder ta
.setRecordKeyFields(props.getProperty(DataSourceWriteOptions.RECORDKEY_FIELD().key()))
.setPopulateMetaFields(props.getBoolean(HoodieTableConfig.POPULATE_META_FIELDS.key(),
HoodieTableConfig.POPULATE_META_FIELDS.defaultValue()))
+
.setMetaFieldsModeFromString(props.getString(HoodieTableConfig.META_FIELDS_MODE.key(),
Review Comment:
Correcting what I told you earlier on this thread: I said the restart case
would work by inheritance — the writer picking up the table's mode when it
states nothing. That is no longer what happens, and I want to be explicit about
the reversal rather than let you discover it in the diff.
After discussing the write path with @danny0405, we settled on the opposite
rule. The writer's resolved mode must simply equal the table's:
| Table mode | Writer's resolved mode | Outcome |
|---|---|---|
| `ALL` | `ALL` (incl. stating nothing) | proceeds |
| anything | equal to the table's | proceeds |
| anything | different from the table's | rejected |
A writer that states nothing resolves to the `ALL` default. That agrees with
an `ALL` table — the overwhelmingly common case, and why nearly every existing
caller is unaffected — and disagrees with every other mode, including `NONE`.
So your restart scenario now **fails loudly at init** instead of inheriting.
Concretely, a streamer restarted against a `COMMIT_TIME_ONLY` table without
`hoodie.meta.fields.mode` in its props gets:
```
hoodie.meta.fields.mode mismatch: table is COMMIT_TIME_ONLY but the writer
does not state one and
so resolves to ALL. Meta columns are physical, so the writer must match the
table -- enabling a
column now would leave earlier commits without it.
Set hoodie.meta.fields.mode=COMMIT_TIME_ONLY on the writer, or change the
table's mode through
hudi-cli.
```
### Why we went this way rather than inheriting
Inheritance works fine at `initTable` — that was never the problem. The
problem is everything downstream of it. The mode is read again by the write
handles, by `HoodieAvroFileWriterFactory#newParquetFileWriter` and
`HoodieSparkFileWriterFactory#newParquetFileWriter`, and each of those derives
it from the `HoodieConfig` it is handed. Of the eight production
`getFileWriter` call sites, **three hold no table config at all**
(`ParquetUtils`, `HoodieNativeLogFormatWriter`, `HoodieNativeCDCFileWriter`).
Nothing at those sites can repeat the inference.
That left two options: thread the table config through the whole factory
chain (public API used by Flink and the Java client), or require the write
config to be correct on arrival. We picked the second. Requiring the statement
is what lets every one of those downstream readers trust the write config
without reconciling it — the guarantee holds because the write is refused
otherwise, not because each site remembers to look it up.
The narrower consideration is that an unstated writer resolves to the `ALL`
default on its own. Inheriting silently would be the difference between writing
all five meta columns and writing one, decided by a fallback the caller never
saw.
One correction to an earlier version of this rule, which a test caught: I
first scoped the must-state-it requirement to selective tables only. That left
`NONE` tables accepting an unstated writer, which resolves to `ALL` and would
populate all five meta columns on a table that has none of them. The rule is
now plain equality of resolved modes, which covers that case and is simpler to
state.
### What this costs, and what it does not
Every writer against a table that is not `ALL` must state its meta-field
settings. That is a real requirement and I do not want to undersell it. Two
things bound it:
- An `ALL` table — the default, and the overwhelming majority — accepts a
writer that says nothing, because such a writer already resolves to `ALL`.
Existing callers against those tables keep working untouched, table services
included (they inherit the caller's config, so the setting propagates without
any change on their side). A `NONE` table already required
`populate.meta.fields=false` on the writer before this PR, so in practice the
new burden falls on the three selective modes.
- A rejected write changes nothing. The table's mode is untouched and no
commit lands. The previous behavior silently narrowed the table and wrote base
files with a null `_hoodie_commit_time` into a table that still advertised
`COMMIT_TIME_ONLY` — incremental queries were then admitted and dropped exactly
those rows. Failing at init is strictly better than that.
### Tests
`TestHoodieStreamerMetaFieldsMode` covers both halves end-to-end through the
streamer:
- `testRestartWithoutRestatingTheModeIsRejected` — your case. Asserts the
throw, and that the rejected run left the mode intact, committed nothing, and
wrote no row with a null commit time.
- `testRestartRestatingTheModeSucceeds` — the migration path: restating the
same mode proceeds, both commits land, and every row across them carries a
commit time.
I verified the first is not vacuous: with the rule disabled it fails with
`expected the writer to be told to state the mode ... expected: <true> but was:
<false>`, and passes with it enabled.
If you think the ergonomic cost of requiring the property is too high for
your use case, I would rather hear it now than after this lands — the
alternative (option 2 above, threading the table config through the factory
chain) is a bigger change but it does remove the requirement.
--
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]
