DanielLeens commented on PR #12443:
URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6310079424
Thanks @SEZ9 for the careful pass, and thanks @hutiefang76 for the fast
pushes. I re-read the diff at `3d6c855f30d0` while preparing my last review, so
here are the concrete locations you asked about, plus a CI update, because my
previous note on CI (still pending) is now out of date.
**F1 / F5 - shared helper and call sites.** The helper is
`ConfigMapPathUtils.quoteInvalidPathKeys` in `seatunnel-api`. It uses
`ConfigUtil.splitPath` as the validity oracle, so keys the parser accepts are
returned unchanged and only rejected keys get `ConfigUtil.quoteString`. It is
called right before every production `Map -> Config` conversion:
- `ReadonlyConfig.toConfig()` (seatunnel-api)
- `ConfigBuilder`: the REST map overload, the config adapter path, variable
backfill, the user variable map and the merged user/system variable map
(seatunnel-core-starter)
- `ConfigShadeUtils.decryptConfig`, the second `parseMap` that used to drop
`^t_nova_.*$` (seatunnel-core-starter, around line 229)
So REST and file loading now go through the same normalization.
**F3 / F2 - docs.** Both are covered in en and zh: `rest-api-v2.md` (REST
body, HOCON-vs-JSON difference, the need to quote reserved keys in HOCON) and
the new section in `introduction/concepts/config.md`. They state that `->`
still splits into nested keys, quotes are stripped, surrounding unquoted
whitespace is trimmed, and that only keys rejected by the path parser become
literal. They also say explicitly that this is not byte-for-byte key
preservation.
**F4 / F6 - tests.** Mapped to your list:
- (a) `ConfigBuilderTest#testInvalidPathKeysSurviveConfigShadeRoundTrip`
runs `^t_nova_.*$`, `a:b`, `${FOO}` and an escaped quote/backslash key through
`decryptConfig(ConfigBuilder.of(map))`.
- (b) and (d) are in the same test, plus `RestUtilTest` (quote/backslash
round trip, standalone `$`, `:` and `${FOO}`).
- (c) top-level regex key: `RestUtilTest` and
`ReadableConfigTest#testToConfigPreservesInvalidPathKey`.
- (e) `RestUtilTest` asserts that `{"a->b":"value"}` becomes the nested
`a.b` and that no literal `a->b` key remains.
- REST integration:
`RestApiIT#testSubmitJobWithHoconFormatAndQuotedLiteralKey`,
`#testSubmitJobWithRegexFieldInJson` and the HOCON decrypt cases.
**F7 - title and modules.** Production changes are in `seatunnel-api` and
`seatunnel-core-starter` only. `seatunnel-engine-server` and `seatunnel-e2e`
contain tests only, so `[Fix][Core]` fits.
**CI update for `3d6c855f`.** The fork `Build` (run 39429936411) has now
completed with a failure, and I looked at the failing jobs:
- `unit-test` on JDK 11 and 17, Ubuntu and Windows: all four fail with the
same single error,
`LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled:248`
(`InvalidClassException`), in `seatunnel-transforms-v2`. That is the failure
tracked in #12929, in a module this PR does not touch.
- `engine-v2-it (11)`: 1 failure in 208 tests,
`BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure:247`
(2 extra checkpoints observed, 3 expected). That is the checkpoint-lock
starvation flake that #12316 targets, and it is also outside this diff.
- `all-connectors-it-4` and two `mysql-cdc-connector-it` jobs were cancelled
after about 2 hours each, which looks like the job time limit rather than a
test result.
None of these touch config parsing, and the REST ITs in `engine-v2-it` did
not fail. That is my reading of the logs, not a green run, so there is still no
passing `Build` for this head. My conclusion is unchanged: no source-side
blocker, and the merge gate is a clean `Build` once #12929 is fixed on `dev`.
At that point, syncing the branch (it is 1 commit behind `dev`) and re-running
only the failed jobs should be enough.
Final approval still needs a committer. Thank you both!
--
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]
