SEZ9 commented on PR #11496: URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5724008488
@NganWave Thanks for the detailed rundown — that matches what I was hoping for. Where things stand from my side: **F2** — Dropping `withFallback(cleanSourceConfig)` in favour of `processVariablesMap` plus `originalResolvedConfigMap.keySet().removeIf(key -> !originalRootKeys.contains(key))` is the right shape. One ask: please add a unit test that passes a `-i` key which does not exist in the job config and asserts it is absent from the final config root, so the leak can't silently come back. **F4 / F5** — Agreed that dropping `System.setProperty` is a reasonable fix for cross-job contamination, and I'm fine with not documenting it as a feature. Since it is still an observable behavior change for anyone who resolved `-i` values via `System.getProperties()`, please add a one-line note to the PR description (and, if you have one, the release-notes/changelog entry) so it's discoverable. No code change needed from me on this point. **F6 / F8** — Good to hear the fail-fast on unbalanced braces/brackets and unterminated quotes is back with tests. I'll verify on the next head. **Remaining (F1, F3, F7)** — Sounds good. For F1 specifically, please pick one behavior for duplicate `-i` keys (last-wins as before, or fail-fast with a clear message), guard against a null parsed value, and cover both in a test. For F3, either allow `[`/`]` in default values or explicitly document the restriction, with a test either way. For F7, an upgrade note covering braces / interior quotes in `-i` values is enough. Also, as you noted, the new items flagged on `00d5018647` (introduced in `0318a671b4`) should land in the same push along with their regression tests. Once that's up I'll do a full re-review of the diff. <!-- streview-comment:1123 --> -- 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]
