SEZ9 commented on PR #11500: URL: https://github.com/apache/seatunnel/pull/11500#issuecomment-6356273710
Thanks @zhangshenghang — sorry for the delay in getting back to you. **Issue 1 (migration sizing):** Good, the per-job `Σ (2 + P_j)` framing plus the explicit caveats about non-aggregate-committer sinks, multi-source/multi-pipeline plans, and "aggregate count ≠ placement guarantee" is the right direction. I'll re-read the en+zh incompatible-changes text in 0961519e9. While you're in that section, the two gaps I flagged earlier still need a sentence each: (a) `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`; and (b) on master-switch / upgrade restore, jobs from `runningJobInfoIMap` are re-evaluated by `preApplyResources()` under the fixed cap and can be marked FAILED even though they ran fine under dynamic slots. Both should be called out in the migration entry. **Issue 2 (`ContainerTestingExtension` fallback):** I have to be straightforward here — the factory-inventory vs global-inventory fallback was not one of the findings in my review of this PR, so I'm not the right person to pick between "fix here" and "move it to the owning work" in this thread. I'd suggest taking that decision with the reviewer who raised it; I have no objection to keeping it out of this default-slot PR, and I agree skip policy shouldn't be broadened quietly as a side effect here. Remaining asks from my review that are still open: 1. **Test coverage for the new default** — both `isDynamicSlot()` assertions load YAMLs that set `dynamic-slot: false` explicitly, and the shared e2e container copies a `seatunnel.yaml` with `dynamic-slot: true`, so nothing actually exercises the flipped default. One assertion on a config without the key would be enough. 2. **`separated-cluster-deployment.md` (en + zh)** — section 4.2 says static is the default but the full `Example` block in 4.3 still ships `dynamic-slot: true`; please align it with the hybrid page. 3. **"(default)" snippets** — pairing `dynamic-slot: false` with `slot-num: 20` reads as if 20 were the default slot count; please either drop `slot-num` from the default snippet or note it's an example. The zh k8s configuration guide also still needs the same rewording as its en counterpart. 4. **PR description** — it still advertises the `setNullToStatementByDataType` dedup in `AbstractJdbcRowConverter`, which isn't in this diff; please drop that line or push the change. Once those land I'm happy to take another pass. <!-- streview-comment:1964 --> -- 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]
