-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[Feature][seatunnel-config] connection configuration uniform and reus… #9116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
duplicate pr #8984 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 9 out of 13 changed files in this pull request and generated no comments.
Files not reviewed (4)
- seatunnel-core/seatunnel-core-starter/src/test/resources/mysql_to_console_by_ref.conf: Language not supported
- seatunnel-core/seatunnel-core-starter/src/test/resources/ref.conf: Language not supported
- seatunnel-engine/seatunnel-engine-client/src/test/resources/fake_to_console_by_ref.conf: Language not supported
- seatunnel-engine/seatunnel-engine-client/src/test/resources/ref.conf: Language not supported
Comments suppressed due to low confidence (2)
seatunnel-engine/seatunnel-engine-core/src/main/java/org/apache/seatunnel/engine/core/parse/MultipleTableJobConfigParser.java:110
- [nitpick] Consider renaming 'refMaps' to 'refConfigs' for consistency with its usage in mergeWithRefConfig and clearer semantic meaning.
private final Config refMaps;
seatunnel-core/seatunnel-core-starter/src/main/java/org/apache/seatunnel/core/starter/utils/ConfigShadeUtils.java:212
- [nitpick] Consider renaming the parameter 'RefConfig' to 'refConfig' to follow standard Java naming conventions.
refMap.forEach((refId, RefConfig) -> {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a uniform connection configuration approach by enabling the reuse of common configuration files via a new ref_config_path and st_ref_key mechanism. Key changes include the addition of refMaps and mergeWithRefConfig logic in the job config parser, updates to the tests to validate the new reference-based configuration parsing, and extensive documentation updates in both English and Chinese.
Reviewed Changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| seatunnel-engine-core/src/main/java/org/apache/seatunnel/engine/core/parse/MultipleTableJobConfigParser.java | Adds refMaps field, ref_config_path handling, and merging logic for plugin and reference configurations. |
| seatunnel-engine-client/src/test/java/org/apache/seatunnel/engine/client/MultipleTableJobConfigParserTest.java | Introduces tests to verify job configuration parsing using st_ref_key and ref_config_path. |
| seatunnel-core-starter/src/test/java/org/apache/seatunnel/core/starter/utils/ConfigShadeTest.java | Tests added for validating the parsing of reference configurations. |
| seatunnel-core-starter/src/main/java/org/apache/seatunnel/core/starter/utils/ConfigShadeUtils.java | Updates config processing to support configuration reference usage when SOURCE key is absent. |
| seatunnel-api/src/main/java/org/apache/seatunnel/api/options/EnvCommonOptions.java | Adds the REF_PATH option to facilitate reference configuration file paths. |
| docs/(zh | en)/concept/(config |
Files not reviewed (4)
- seatunnel-core/seatunnel-core-starter/src/test/resources/mysql_to_console_by_ref.conf: Language not supported
- seatunnel-core/seatunnel-core-starter/src/test/resources/ref.conf: Language not supported
- seatunnel-engine/seatunnel-engine-client/src/test/resources/fake_to_console_by_ref.conf: Language not supported
- seatunnel-engine/seatunnel-engine-client/src/test/resources/ref.conf: Language not supported
Comments suppressed due to low confidence (1)
seatunnel-core/seatunnel-core-starter/src/main/java/org/apache/seatunnel/core/starter/utils/ConfigShadeUtils.java:218
- When the configMap does not contain the 'source' key, the code falls back to constructing and returning a refMap. Please verify that this behavior is intentional and does not inadvertently alter the expected configuration structure.
return ConfigFactory.parseMap(refMap);
| this.seaTunnelJobConfig = ConfigBuilder.of(Paths.get(jobDefineFilePath), variables); | ||
| this.envOptions = ReadonlyConfig.fromConfig(seaTunnelJobConfig.getConfig("env")); | ||
|
|
||
| String refPath = envOptions.getOptional(EnvCommonOptions.REF_PATH).orElse(null); |
Copilot
AI
Apr 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handling of the ref_config_path is inconsistent between constructors; one passes variables to ConfigBuilder while the other uses null. Consider standardizing the parameter usage to ensure consistent behavior across instances.
|
This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs. |
|
This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request. |
thanks to
@hailin0 https://github.com/hailin0.
@liunaijie https://github.com/liunaijie
@wuchunfu https://github.com/wuchunfu
@litiliu https://github.com/litiliu
reference the design:
#8945 (comment)
Purpose of this pull request
[Feature] connection configuration uniform and reuse in job config
impl: #8941
Does this PR introduce any user-facing change?
Yes
support
ref_config_pathin env, and usest_ref_keyto merge common config.update doc file:
How was this patch tested?