DanielLeens commented on PR #12443:
URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-5882039178
Thanks @hutiefang76 and @SEZ9 for closing out the last open threads on this.
I've read the full exchange since my last review of `ddca53b7a` (the current
head):
- I independently pulled the raw `docs/en/engines/zeta/rest-api-v2.md` at
`ddca53b7a` and confirmed the note is exactly where you said: it accurately
states that keys failing HOCON path parsing (regex field names, reserved
characters like `:`/`${...}`) are treated literally, while keys that do parse
still go through the existing `->`/quote/whitespace normalization — matching
the F3 clarification above. Good to have that written down explicitly.
- The F7 title call (`[Fix][Core]`) matches the final diff footprint
(`seatunnel-api`, `seatunnel-core-starter`, plus engine-server/E2E tests and
both docs), so that seems right to me too.
**One correction to my own last review, since it's now stale:** when I
approved `ddca53b7a`, the fork's Build run was still `queued` with no signal
either way. It has since finished, and it's currently **failing** — 10 jobs
red: `all-connectors-it-2` (JDK 8 & 11), `all-connectors-it-6` (JDK 8 & 11),
`all-connectors-it-7` (JDK 8 & 11), `transform-v2-it-part-1` (JDK 8),
`engine-v2-it` (JDK 11), and `paimon-connector-it` (JDK 8 & 11). None of these
modules touch the files this PR changes (`seatunnel-api` config-path utils,
`ConfigBuilder`/`ConfigShadeUtils`, `ReadonlyConfig`, and the REST/config
tests), and the fork is now 38 commits behind `dev` (up from 18 at my last
check), so this has the shape of pre-existing/environmental flakiness the
branch hasn't picked up fixes for yet rather than something introduced by this
diff. I wasn't able to pull a specific failing assertion for each of the 10
jobs from the logs, so I won't claim more precision than that — but given the
breadth a
nd total lack of overlap with the changed files, I'd recommend merging latest
`dev` into this branch and re-running CI before relying on this run's result.
With that correction noted, my source-level conclusion from the last round
is unchanged: no outstanding blockers in the diff itself, all four production
`Map`→`Config` choke points are guarded and tested, and the docs are in place.
Once a fresh CI run off a synced `dev` comes back clean, this looks ready to
merge from my end. Great collaborative back-and-forth on this one, @hutiefang76
— thanks for the patience through all the rounds.
--
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]
