DanielLeens commented on PR #11500: URL: https://github.com/apache/seatunnel/pull/11500#issuecomment-6143687146
Thanks for the detailed pass, @SEZ9 - and apologies to @zhangshenghang: my last review on this head (`b4cadf311b`) said "Ready to merge" with only my one Low test note, and that was too generous. I re-checked each of the six findings in @SEZ9's review against the pinned head, and they hold. Please treat my previous conclusion as superseded by "Ready to merge after fixes". The source line itself (`ServerConfigOptions.java:529`) is still correct; the gaps are in coverage and in the documented blast radius. Verification on `b4cadf311b`: - **SEZ9 Issue 1 (no test pins the new default): confirmed, and it subsumes my earlier Low item.** The e2e base `seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/resources/seatunnel.yaml:26` still sets `dynamic-slot: true`, so CI never runs the shipped default. A unit assertion on `new SlotServiceConfig().isDynamicSlot()` plus a parse of a YAML without a `slot-service` block would close it. - **SEZ9 Issue 2 (`-e local`): confirmed.** `ClientExecuteCommand.java:91` loads the shipped `config/seatunnel.yaml`, and `createServerInLocal()` (lines 335-372) only overrides cluster role, mode, port auto-increment and HTTP, never the slot service. So a local run now gets `2 x availableProcessors()` fixed slots. Either force dynamic slots there when the user did not pass `-Dseatunnel.config`, or add a local-mode bullet to `incompatible-changes.md` (en + zh). - **SEZ9 Issue 3 (restore path): confirmed.** `CoordinatorService.java:1160` enqueues a `PendingJobInfo(RESTORE, ...)`, and the scheduler at lines 439-465 calls `completeFailJob()` for any not-enough-resources result under `REJECT`, without distinguishing RESTORE from SUBMIT. The migration entry only describes the submit path. - **SEZ9 Issue 4 (half-flipped separated-cluster page): confirmed.** `docs/en/engines/zeta/separated-cluster-deployment.md:164` (and the zh counterpart) still has `dynamic-slot: true`, while the hybrid page was flipped at line 104. - **SEZ9 Issue 5 (`slot-num: 20` under a "(default)" heading): confirmed**, Low. - **SEZ9 Issue 6 (PR description advertises a JDBC change that is not in the diff): confirmed.** The PR body still has "### 2. [Connector-V2] Remove duplicate `setNullToStatementByDataType`", and the diff has no `connector-jdbc` file. Please trim the body (and the eventual squash message). So the blockers before merge are SEZ9 Issues 1-4 (all Medium), plus the Low items 5 and 6 as recommended fixes. Nothing else changed in my assessment of the core change. On CI: `Build` on this head is still `action_required` with no fork run, so once the fixes are pushed, please make sure Actions can run on the fork so the new test and the e2e config flip get exercised. Happy to re-review after the update, and thanks again for the patience, @zhangshenghang! -- 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]
