DanielLeens commented on PR #11947:
URL: https://github.com/apache/seatunnel/pull/11947#issuecomment-5391515229

   Follow-up on my previous review: I wrote it about three minutes into this 
head's (`d6e24425b582`) CI run, while `Build` was still in progress. That run 
has since finished, and it is now `FAILURE` (fork run `DanielLeens/seatunnel` 
run `32638154468`), so I want to correct the record rather than let "Ready to 
merge" stand unqualified.
   
   I pulled the failed job logs (`engine-v2-it` shards 8 and 11) rather than 
just reading the check name. Both shards fail the same way, and it is not a 
flake:
   
   ```
   [ERROR] SeaTunnel : E2E : Engine : Base .................... FAILURE [48:57 
min]
   ...
   org.apache.seatunnel.api.table.factory.FactoryException: ErrorCode:[API-06], 
ErrorDescription:[Factory initialize failed] - Unable to create a source for 
identifier 'FakeSource'.
   Caused by: java.lang.NoClassDefFoundError: 
org/apache/seatunnel/connectors/cdc/base/source/BaseChangeStreamTableSourceFactory
   Caused by: java.lang.ClassNotFoundException: 
org.apache.seatunnel.connectors.cdc.base.source.BaseChangeStreamTableSourceFactory
   ```
   
   This is not confined to the new `MysqlCDCClusterFailoverIT` — it takes down 
essentially every pre-existing test in `connector-seatunnel-e2e-base` that 
submits a plain `FakeSource` job (`PendingJobsRestIT`, 
`ClusterFaultToleranceIT`, `RestApiIT`, `SeaTunnelSlotIT`, 
`SplitClusterFaultToleranceIT`, `SplitClusterPendingJobLifecycleFailoverIT`, 
`TextHeaderIT`, `SlotRatioAllocateStrategyIT`, `SystemLoadAllocateStrategyIT`, 
`TimerFlushIT`), 66 errors in shard 11 alone. `FakeSource` has no legitimate 
runtime dependency on `connector-cdc-base`'s 
`BaseChangeStreamTableSourceFactory`, so this reads as a classpath/shading 
regression in the module, not a test-logic problem in the new file.
   
   Given this PR's own diff adds `connector-cdc-mysql` (regular + test-jar) as 
a new test-scope dependency to this module's `pom.xml` so the new test can 
reuse `MySqlContainer`/`MySqlVersion`, that's the most likely source of the 
break — e.g. a shaded/relocated class from `connector-cdc-base` ending up 
missing or in conflict once `connector-cdc-mysql`'s own dependency graph is 
pulled into this module's test classpath. I have not proven the exact 
mechanism, but the "unrelated pre-existing FakeSource tests break module-wide" 
signature points at the dependency addition, not at test logic in 
`MysqlCDCClusterFailoverIT` itself.
   
   This needs to be root-caused and fixed before merge — it is a real, 
PR-caused break of unrelated existing coverage, not something to rerun past. 
I'd suggest checking whether the new test-jar dependency can be scoped more 
narrowly (e.g. excluding transitive relocations that collide with what this 
module already provides) or whether a `connector-cdc-mysql` test-jar is even 
necessary versus duplicating the small pieces of 
`MySqlContainer`/`MySqlVersion` needed here.
   


-- 
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]

Reply via email to