-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[Improve] restruct connector common options #8634
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
|
cc @hailin0 |
541e429 to
1afc808
Compare
|
|
||
| public static OptionRule getEnvOptionRules() { | ||
| @AutoService(Factory.class) | ||
| public class EnvOptionRule implements Factory { |
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.
Make this class implement Factory. Then we can also use SPI to get the env config parameters.
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 env is only one, why we need SPI?
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.
If use Factory. we can use this code to verify parameter.
OptionRule optionRule = ServiceLoader.load(Factory.class, classLoader).filter("xxxxx").getEnvOptionRules();
ConfigValidator.of(context.getOptions()).validate(optionRule);
If not implement Factory. the behavior is like this.
OptionRule optionRule;
if( is connector ){
optionRule = ServiceLoader.load(Factory.class, classLoader).filter("xxxxx").getEnvOptionRules();
} else if ( is env ) {
optionRule = EnvOptionRule.getEnvOptionRules();
}
ConfigValidator.of(context.getOptions()).validate(optionRule);
I am both Ok. but looks implement Factory looks have a unified style.
1afc808 to
34137a0
Compare

Purpose of this pull request
subtask of #8576
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note.