DanielLeens commented on PR #12944: URL: https://github.com/apache/seatunnel/pull/12944#issuecomment-6362880408
Thanks @SEZ9 for the thorough pass on `ca971f98`, and thanks @Nikk8091 for the clear follow-up notes. No new commit has landed since my last review, so this is a short reply to the open points rather than a re-review. I re-checked the head source for each item. **Agreed, still the only hard blocker (SEZ9 Issue 1, same as my Issue 1).** `ConfigMapPathUtils` is imported at `MetadataProviderManager.java:27` and used at `:107` / `:348`, but it does not exist in this head or on `dev`; it only comes from #12443, which is still open. One small update to the facts: #12443's head is currently `509fddab`, not `3d6c855f` (the combined-tree base used in the PR description) and not `ab771cf9`. So the combined-tree results are indeed stale. Once #12443 merges, please rebase onto that `dev` and re-run `MetadataProviderManagerTest` plus `MetalakeIT.testDataSource` in CI on the new head. The Build check on this head is currently `action_required` (the fork workflow is not enabled), so please enable Actions in your fork or push after the rebase so that a real run is attached. **Issue 4 (docs say job-wins, code is datasource-wins): +1, verified.** `docs/en/introduction/concepts/metadata-spi.md:57` and `:373` (and `docs/zh/.../metadata-spi.md:57` / `:372`) say job-level parameters take precedence, while `mergeConfig` (`mergedMap.put(key, value)` for each datasource entry, and its Javadoc) lets the datasource value override. The new test pins the code behavior, so I agree the docs are the side that is wrong. This is a pre-existing doc bug, so I would fix the en/zh wording in this PR (two lines each) rather than leave a test and a doc that contradict each other. Please do not change the code-side precedence here, since that would alter behavior for existing metadata users. **Issues 3 and 5 (document the key contract): +1, non-blocking.** A short Javadoc note on `mergeConfig` / `resolveDataSourceConfigs` is enough: `root().unwrapped()` drops key quoting, re-parsing goes through the helper, keys rejected by the path parser are kept literal, and valid path forms such as `a->b` keep their existing nested meaning. I would keep provider-key behavior as is (valid paths stay paths) because quoting every provider key would be a different compatibility change; the docs note is the right scope. **Issue 2 (avoid the full-job round trip): reasonable, but I would treat it as optional.** The `unwrapped()` -> `parseMap` rebuild of `env` / `transform` is existing behavior, and this PR's job is to stop it from throwing `BadPath` on literal keys. A cheap, low-risk improvement is to return `seaTunnelJobConfig` unchanged when no connector actually changed; replacing only the `source` / `sink` lists via `withValue` is cleaner but touches more of the method. I am fine with either, or with leaving it for a follow-up, as long as the author notes the choice. **Issue 6 (partly): one correction.** Only the two `enabled=false` rows of `testSchemaKeysWithoutDatasource` return early at `:64-66`. The two `enabled=true` rows have no datasource id, but still go through `resolveDataSourceConfigs` and reach the rebuild at `:107` (the engine gate is separate), so they do exercise the changed line. Splitting the disabled rows into a plainly named test and keeping the PR description's coverage claim accurate is a nice-to-have. **Issue 7 (tautological fixture assertions): agreed, low.** Either snapshot the provider map before the call and compare the whole map afterwards to prove non-mutation, or drop those few lines. So the summary from my side: Issue 1 (prerequisite plus real CI on the rebased head) blocks; the docs precedence fix is the one follow-up I would like included; the rest are small and non-blocking. I will re-review as soon as the rebased head is pushed. Thanks again for the careful work on this. -- 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]
