hesam-oxe commented on PR #11077:
URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5692131695
@SEZ9 @DanielLeens Thanks both. Two things below: a short note on the F1-F8
checklist, and the CI evidence for the current head `ef2bb095`.
## On the checklist
I agree with @DanielLeens' 09-15 mapping - F1-F8 were each re-verified
against this head rather than carried forward from the pre-`a9d8464` round. For
the two items still listed as HIGH:
**F1 (destination-key collisions).** `DestinationKey.equals()/hashCode()`
(`MultiTableSink.java:621-670`) folds in `sink.getClass()` plus the
connector-supplied physical identifier, falling back to raw object identity
when either side has none. Two different connector classes that resolve to the
same identifier string therefore get distinct keys and distinct writers, so one
table's rows cannot be routed through another sink's writer. Pinned by
`testSamePhysicalIdentifierDoesNotShareAcrossConnectorClasses`.
**F2 (snapshot fan-out / restore-time union).** The snapshot is persisted
under one canonical identifier, and `getRestoredState()`
(`MultiTableSink.java:392-404`) merges legacy per-alias checkpoints by content
rather than by position, so a pre-PR checkpoint restores into exactly one
shared writer instead of N duplicated copies. Pinned by
`testSharedWriterRoundTripRestoresOneCanonicalState` and
`testRestoreMergesStateFromAllAliasedTables`.
F3 is covered by `validateSharedDestinationSchemas()`
(`MultiTableSink.java:129-161`), which fails fast at construction on divergent
`CatalogTable` schemas across aliases; the upgrade-time behaviour change is
called out in the PR's Release note section. F4/F6 are documented in
`docs/{en,zh}/developer/sink-connector-development.md`, including the changed
`restoreWriter` contract for connector implementers. F8's `@param`/`@return`
tags are present on `getDestinationKey`.
For F5 and F7 I cannot reproduce them on this head: both `createWriter` and
`restoreWriter` call `proxyContexts.put(...)` unconditionally per alias, so
there is no first-alias-only registration and no `containsValue` gate anywhere
on that path; and writer creation happens outside `computeIfAbsent`, so the
checked `IOException` propagates to the single outer `catch (IOException
error)` instead of being wrapped in an unchecked `RuntimeException`.
Happy to walk through any of these line by line if a finding still does not
look resolved from your side - I would rather close the gap than argue the
checklist.
## On CI
The `dev` sync did what it was meant to: the MinIO `404` is gone and
`PaimonWithS3IT` now passes. Rather than re-run blind, I pulled the full logs
for the remaining `Build` failure on `ef2bb095` (fork run `34826109614`): **94
jobs - 74 success, 7 failure, 2 cancelled, 11 skipped.**
**`engine-v2-it (8, 11)`** fails on
`CheckpointCoordinatorFailoverIT.testStreamJobFailsAfterCheckpointTriggerDispatchFailure:826`:
```
Expected the job failure to be attributed to the checkpoint coordinator's
CHECKPOINT_INSIDE_ERROR path, but got:
CheckpointException: Checkpoint notify complete failed
Caused by: IllegalArgumentException: can't find task group address from
taskGroupLocation: TaskGroupLocation{jobId=..., pipelineId=1,
taskGroupId=1}
at JobMaster.queryTaskGroupAddress(JobMaster.java:1067)
```
The same job in the same workflow **run on `dev` itself** - run
`34935118103`, head `f4a9665e84`, containing no part of this PR - fails the
identical test at the identical line with the identical assertion message. It
is a task-group-address race in `JobMaster`, not a sink-side regression. That
baseline run is red on `engine-v2-it (8)`, `engine-v2-it (11)`,
`all-connectors-it-2 (8, 11)`, `all-connectors-it-7 (8)`,
`transform-v2-it-part-1 (8)`, `paimon-connector-it (8)` and `kudu-connector-it
(8)` - i.e. the same set this head is red on.
`engine-v2-it` also reports
`BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure:247`:
```
expected at least 3 additional checkpoints to complete during the 90s
sustained backpressure window, only observed 1 (samples=[1, 2, 2, 2, 2, 2,
2, 2, 2])
```
That is a wall-clock assertion over a 90s window on a shared runner. Both
tests are FakeSource -> in-memory/slow-sink pipelines: neither routes through
`MultiTableSink`, and neither file appears in this PR's diff (15 files, all
under `seatunnel-api/.../multitablesink/`, `connector-file-base`, and `docs/`),
so the changed code path is not exercised by them.
**`all-connectors-it-2 (8, 11)` / `all-connectors-it-7 (8, 11)`** are the
pre-existing flakes already tracked earlier in this thread, and both fail
identically on the `dev` baseline above.
**`jdbc-connectors-it-part-1 (8)` and `kudu-connector-it (11)`** were
*cancelled*, not failed: `The job has exceeded the maximum execution time of
2h0m0s` and `1h30m0s` respectively.
**`Run / Dead links`** reports exactly one dead link:
```
FILE: ./docs/en/ai-cli/benchmark.md
[x]
https://github.com/apache/seatunnel/blob/dev/seatunnel-cli/benchmark/README.md
ERROR: 1 dead links found!
[x] ... -> Status: 503
```
A `503` is GitHub throttling the link checker from the runner, not a broken
link - that URL returns `200` when fetched directly, and
`seatunnel-cli/benchmark/README.md` exists on `dev`. The referencing file,
`docs/en/ai-cli/benchmark.md`, is not part of this diff either.
**Net:** nothing in the `Build` failure traces back to this change, and the
`dev` baseline is red on the same jobs in the same way. The MinIO fix from
#12287 that the sync pulled in did its job. If a maintainer would like a clean
green board before merge, re-running `engine-v2-it` and `Dead links` off-peak
should be enough - say the word and I will trigger 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]
