DanielLeens commented on PR #12567:
URL: https://github.com/apache/seatunnel/pull/12567#issuecomment-6158026220
Thanks @Asthenia0412, and thank you for tracing Issue 3 so carefully. Your
reading is right: the restart position is the part that needs to change. I
re-checked head `c490b75`: the diff against my last review at `e18f31c` is
identical (the four newer commits are empty "retrigger CI" or sync commits), so
my 10-10 review still applies as written and there is no new code to re-review
yet.
To answer your question: please sketch it right here on this PR. `#12698` is
an unrelated, already merged ActiveMQ PR, so there is nothing to coordinate
there. Here is the smallest shape I would try (Option A from my review):
1. `IncrementalSplitAssigner#addSplits` (`:184-199`): stop reading
`incrementalSplit.getTableStartupOffsets()`. Restore
`tableWatermarks.put(tableId, startupOffset)` exactly as on dev, so every
restored table gets the newest position its old split delivered, and the new
split still starts at the minimum across splits.
2. `IncrementalSplitAssigner#createIncrementalSplit` (`:289-324`): keep what
you already have, shipping `tableWatermarks` of the captured tables as
`tableStartupOffsets`. That map is the per-table boundary the fetcher needs.
3. `IncrementalSourceStreamFetcher#configureFilter` (`:338-359`): keep as
is, it already seeds the exactly-once filter from `tableStartupOffsets`.
4. Drop the reader-side tracking (the emitter change and the per-table map
in `IncrementalSplitState`), and keep only the monotonic guard on the
split-level offset. This also removes my Issues 2 and 3.
5. Add one assigner-level test: two restored splits, one with an older
startup offset and one newer, a table in each. Assert the new split starts at
the older one, not earlier, and that each table's `tableStartupOffsets` entry
is its own split's offset.
Once that is pushed I will re-review the full head in one pass.
On CI: the apache-side `Build` check on this head is `action_required`
("Workflow run detection failed"), and I do not see a `Build` run in your fork
for this branch, so nothing has executed yet. Pushing more empty commits will
not help until the fork's workflows run. Could you open the Actions tab of your
fork and make sure workflows are enabled (the "I understand my workflows, go
ahead and enable them" button if it shows), then push the fix commit, ideally
on top of the latest `dev`? If the `Build` run still does not appear after
that, leave a comment with a screenshot of the Actions tab and we will dig
further.
This is a static review; I did not execute the PR.
--
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]
