bube commented on code in PR #10109:
URL: https://github.com/apache/paimon/pull/10109#discussion_r4387411027
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/FlinkConnectorOptions.java:
##########
@@ -483,6 +496,20 @@ public class FlinkConnectorOptions {
+
"${UID_PREFIX}_${TABLE_NAME}_${USER_UID_SUFFIX}. If the uid suffix is not set,
flink will "
+ "automatically generate the operator
uid, which may be incompatible when the topology changes.");
+ public static final ConfigOption<Boolean>
SINK_OPERATOR_UID_COVER_ALL_OPERATORS =
+ key("sink.operator-uid.cover-all-operators")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription(
+ "If true, 'sink.operator-uid.suffix' also names
every other operator a streaming "
+ + "write adds: the row conversions, the
local merge, the compaction operators, "
Review Comment:
Thanks for the review. I went with your second option and scoped the options
instead of covering CDC.
Covering only the single-table `CdcSinkBuilder` route would still leave the
option's promise false. `--table_conf` copies the keys into every table of a
database sync, so users can turn it on there too. Covering the database sync
needs a uid shape for operators that belong to no single table: the shared
`Side Output`, the keyed `Schema Evolution`, the multiplexed Convert and, in
combined mode, the Writer and Global Committer of `FlinkCdcMultiTableSink`.
Those last two hold state and have no uid today. Turning the option on would
drop their state on the first restore, which is the kind of migration you asked
us not to recommend. I don't think that belongs in this PR.
So:
- The options are now `sink.operator-uid.cover-table-sink-operators` and
`source.operator-uid.cover-table-source-operators` (c815cc4). Their
descriptions and the guide say the CDC sync actions and `BATCH`-only operators
are not covered.
- The `*.operator-uid.suffix` descriptions now say the suffix alone does not
keep ids stable, and point at the new options. All four link the migration
guide, which is also shorter (781072b).
Two things I checked on the way (e2797de):
- **Source state.** The only source entry the migration drops is the split
monitor's, on the exactly-once consumer route. That route always has a consumer
ID, and after `flink stop` the consumer holds the same next snapshot as the
monitor, so the restored reader neither skips nor re-reads anything. A new
ITCase shows this on Flink 1.20 and 2.2. A bounded read with dedicated split
generation would re-read its scan, so the guide says to let such a job finish
first.
- **The `end` sink for SQL users.** For a `DataStreamSinkProvider` the
planner leaves Paimon's uids alone in every `table.exec.uid.generation` mode. A
SQL-level test pins this.
If CDC coverage is wanted later, it would go under a new key, so nobody who
turned these options on sees ids move on upgrade.
I also rebased onto current master. Does the new name work for you?
--
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]
