bigdataMS commented on PR #4246: URL: https://github.com/apache/flink-cdc/pull/4246#issuecomment-5189887569
Thanks for the update @ThorneANN, and thanks for cleaning up the commit history. I took a close look at #4409 before replying, and I believe the two changes are orthogonal rather than overlapping — I'd hate to see this one dropped on that basis. **#4409 answers "which tables should I subscribe to?"** It introduces the `ObjectIdDiscoverer<T>` / `TableDiscoverer` SPI plus a JDBC implementation, so the subscription list can be read dynamically from a metadata table or a custom query. All 11 files land under `flink-cdc-common/.../source/discover/`. **#4246 answers "a new table showed up — how do we start reading it?"** Specifically: pick it up mid-binlog *without* running a snapshot, so only forward increments are emitted. Concretely: - As far as I can tell from searching master, `TableDiscoverer` currently has **no callers outside its own package** — the SPI is in place but not yet wired into any source, so it doesn't change runtime behaviour for MySQL CDC today. - Its configuration surface (`table.discoverer.jdbc.*` under `source:`) targets the YAML pipeline. The use case @J9527H and I have is the DataStream API with a custom deserializer. - It doesn't define snapshot semantics at all. Even once a source consumes it, newly discovered tables would presumably go through the existing `scan.newly-added-table.enabled` path, which *does* snapshot. Skipping the snapshot is exactly the point of this PR: for a table that has been live for months, we only want increments from now on, and we backfill history separately through a bounded job. If anything the two compose nicely later: a discoverer decides *what* to add, and binlog-only mode decides *how* it gets read. On the current state of this PR — I have a branch rebased onto master (no conflicts) that addresses the outstanding review feedback: 1. Dropped the custom `.` / `\.` remapping and the `TableIdRouter.convertTableListToRegExpPattern` call, so `table.include.list` keeps the connector's standard regex semantics in both modes (@yuxiqian's point). This also removes the need to touch `TableIdRouter` at all. 2. Added the missing stream-only startup-mode validation for `scan.binlog.newly-added-table.enabled`. 3. Fixed the regex examples in `MySqlSourceOptions` and the EN/ZH docs (`db\.*` → `db\..*`, `db\.user_\.*` → `db\.user_.*`), plus three non-standard patterns in the ITCase. 4. Re: Copilot's comment about a redundant check in `MySqlSnapshotSplitAssigner` — the current version of this PR doesn't touch that file, so there's nothing to remove. `spotless`, compile and `MySqlTableSourceFactoryTest` pass locally. The ITCase needs Docker, which I can't run in my current environment, so I'd rely on CI for that. @ThorneANN if you'd still rather not continue, I'm happy to either open a PR against your branch so the changes land under #4246 with your authorship, or open a superseding PR keeping you as the commit author — whichever you prefer. @yuxiqian @lvyanquan could you weigh in on whether this feature is still wanted independently of #4409? If the maintainers' view is that the discoverer SPI is the intended long-term direction for this use case, I'd rather know now than keep pushing on it. -- 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]
