SEZ9 commented on PR #11500: URL: https://github.com/apache/seatunnel/pull/11500#issuecomment-6258668230
Thanks for the follow-up commit. Looking at head `a655ebd7b3` against my previous review at `b0101f3fc3`: the new commit only touches the shared E2E infrastructure (skipping suites left without any enabled container), while the production change, the three `seatunnel.yaml` templates, the docs and the k8s e2e fixture are unchanged. That means the points from the last round are still open, so here is where each one stands and what I'd like to see: - **PR11500-F1 (Test, MEDIUM)** – Still nothing exercises the new default. Both `isDynamicSlot()` assertions load YAMLs that set `dynamic-slot: false` explicitly, and the shared e2e container still copies a `seatunnel.yaml` with `dynamic-slot: true`. Please add a config test that loads a YAML *without* the `dynamic-slot` key and asserts `isDynamicSlot()` is `false`, and either let at least one e2e fixture run on the default (static) slots or explain why the suite should keep running on dynamic slots. - **PR11500-F2 (Compatibility, MEDIUM)** – `seatunnel.sh -e local` now inherits the `2 x availableProcessors()` cap from the flipped `config/seatunnel.yaml`, so jobs with parallelism above `2 x CPU - 2` fail at submit with `NoEnoughResourceException`. Please either keep local mode on dynamic slots or call this out explicitly in the migration entry with the `slot-num` / `dynamic-slot: true` workaround. - **PR11500-F3 (Compatibility, MEDIUM)** – The master-switch / upgrade restore path re-evaluates jobs from `runningJobInfoIMap` via `preApplyResources()` under the fixed cap, so a job that ran under dynamic slots can be marked FAILED on restore after upgrading. Please extend the entry in `docs/en/introduction/concepts/incompatible-changes.md` to cover this and recommend setting `slot-num` / `dynamic-slot` before rolling the upgrade. - **PR11500-F4 (Docs, MEDIUM)** – `separated-cluster-deployment.md` (en + zh) is still half-flipped: section 4.2 says static is the default but the full `Example` block in section 4.3 still ships `dynamic-slot: true`. Please align it with the hybrid page. - **PR11500-F5 (Docs, LOW)** – The "(default)" static-slot snippets pair `dynamic-slot: false` with `slot-num: 20`, which reads as if 20 were the default slot count; please either drop `slot-num` from the default snippet or note that the default is `2 x CPU`. The zh k8s configuration guide also still needs the same rewording as the en one. - **PR11500-F6 (Docs, LOW)** – The PR description / commit message still advertises the removal of a duplicate `setNullToStatementByDataType` in `AbstractJdbcRowConverter`, which is not in this diff. Please either add that change or drop it from the description. Once F1–F5 are addressed (F6 is just a description cleanup) I'm happy to take another look. If you'd rather split any of the compatibility items into a follow-up, let me know which ones and I'll note that on the migration entry review. <!-- streview-comment:1859 --> -- 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]
