SEZ9 commented on PR #11500: URL: https://github.com/apache/seatunnel/pull/11500#issuecomment-6282140652
Thanks for the update. Following up on the open points from the earlier review, nothing in the thread so far addresses them, so here is what is still needed before this can move forward: **Test coverage for the new default (PR11500-F1)** Both existing `isDynamicSlot()` assertions load YAMLs that set `dynamic-slot: false` explicitly, and the shared e2e container copies a `seatunnel.yaml` with `dynamic-slot: true`, so the suite still runs entirely on dynamic slots. Please add a unit test that loads a config *without* the `dynamic-slot` key and asserts the resolved value is `false`, and either flip the e2e `seatunnel.yaml` or add one e2e path that exercises the static default (ideally including a job that hits `NoEnoughResourceException` under the fixed cap). **Local mode behaviour (PR11500-F2)** With the shipped `config/seatunnel.yaml` flipped, `seatunnel.sh -e local` now inherits a `2 x availableProcessors()` slot cap, and jobs whose parallelism exceeds `2 x CPU - 2` fail at submit. Please either keep local mode on dynamic slots (or a larger cap) or add an explicit local-mode paragraph to the migration entry explaining the new limit and how to raise `slot-num` / re-enable `dynamic-slot`. **Upgrade / restore path (PR11500-F3)** Jobs restored from `runningJobInfoIMap` are re-queued and re-evaluated by `preApplyResources()` under the fixed cap, so a job that ran fine on dynamic slots can be marked FAILED after upgrading. Please extend `docs/en/introduction/concepts/incompatible-changes.md` to call this out and recommend setting `dynamic-slot: true` (or a sufficient `slot-num`) *before* the rolling upgrade. **separated-cluster-deployment.md half-flipped (PR11500-F4)** Section 4.2 says static is now the default, but the full `Example` block in section 4.3 still ships `dynamic-slot: true` (both en and zh). Please align it with the hybrid page. **Snippet wording (PR11500-F5)** 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 add a note that the default is `2 x availableProcessors()`. The zh k8s configuration guide also still needs the same rewording as its en counterpart. **PR description (PR11500-F6)** The description/commit message mentions removing a duplicate `setNullToStatementByDataType` in `AbstractJdbcRowConverter`, but that change is not in the diff. Please either add it or remove it from the description so the squash commit is accurate. Once these are in, I'm happy to take another look. <!-- streview-comment:1890 --> -- 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]
