SEZ9 commented on PR #11500: URL: https://github.com/apache/seatunnel/pull/11500#issuecomment-6245864311
Thanks for the follow-up on `b0101f3fc3`. The merge conflict is resolved and the `engine-k8s-it` regression has a targeted fix in `03a8ae2a1d` (`slot-num: 8` pinned in `custom_config/seatunnel.yaml`). Here is where my earlier points stand: **F1 – coverage of the new default.** Does `custom_config/seatunnel.yaml` set `dynamic-slot` explicitly? If it does, the k8s lane still does not exercise the unset-default path. Also, do the two `isDynamicSlot()` assertions still only load YAMLs with an explicit `dynamic-slot: false`, and does the shared e2e container still copy a `seatunnel.yaml` with `dynamic-slot: true`? If the latter is intentional, a short comment explaining why that suite stays dynamic would be enough. **F2 – `seatunnel.sh -e local`.** Local mode inherits the `2 x availableProcessors()` cap from the flipped `config/seatunnel.yaml`. Does the migration entry now call this out and point users to `slot-num` / `dynamic-slot: true`? **F3 – master-switch / upgrade restore path.** Jobs restored from `runningJobInfoIMap` are re-evaluated by `preApplyResources()` under the fixed cap, so a job that ran under dynamic slots can be marked FAILED on restore. Has this been added to the incompatible-changes entry so operators size `slot-num` before upgrading? **F4 – separated-cluster-deployment.md (en + zh).** Section 4.2 says static is the default while the `Example` block in 4.3 still shipped `dynamic-slot: true`. Is it now aligned with the hybrid page? **F5 – `(default)` snippets with `slot-num: 20` / zh k8s guide.** Either drop `slot-num: 20` from the "(default)" snippets or label it as an example value, and apply the same rewording to the zh k8s configuration guide that the en version received, if not already done. **F6 – PR description.** Does the description still mention the duplicate `setNullToStatementByDataType` removal in `AbstractJdbcRowConverter`? That change is not in the diff, so please either trim the description or add the change if it was intended. Since CI for this head has only just been queued, I will wait for it to finish before a final pass. Happy to re-review quickly once the points above are confirmed or pushed. <!-- streview-comment:1847 --> -- 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]
