[FLINK-3904] GlobalConfiguration doesn't ensure config has been loaded #2123
Conversation
mxm
force-pushed
the
FLINK-3904
branch
2 times, most recently
from
bceca69
to
4f81995
Compare
|
This problem sounds like it might cause very hard to debug problems. |
|
Looks like a good fix for now. I would eventually really like to get rid of the In the end, the |
|
That's odd. I was working on exactly these changes and have just pushed them (without seeing your comment before). |
|
Brief summary of changes:
|
| protected static void loadGlobalConfigParams() { | ||
| int maxSamples = GlobalConfiguration.getInteger(ConfigConstants.DELIMITED_FORMAT_MAX_LINE_SAMPLES_KEY, | ||
|
|
||
| protected static void loadConfigParameters(Configuration parameters) { |
There was a problem hiding this comment.
I think this is a public API change. The class is annotated with @Public and the method is protected, which means that some users might have extended this input format and rely on this method. What do you think?
There was a problem hiding this comment.
According to the plugin which checks API changes, it is not breaking. I don't know whether this is a shortcoming of the plugin or actually ok. We can still keep the old method.
|
Very good work Max! The changes look good to me and I think it's much cleaner this way. The Regarding the other "API breaking" changes I'm undecided right now. I fully agree that they are not documented and hence should be fine to remove. On the other hand, the code is not expensive to maintain and if someone is relying on loading of multiple YAML files for example, that will be hard to debug for the user. I feel like it might make sense to get more community feedback before merging this for 1.1 release. What do you think? |
|
Thanks for the review! Although the changes in Regarding the other changes in behavior (removing XML loading, only loading from one flink-conf.yaml file instead of all Yaml/Xml files found): All these changes are in line with our documentation which doesn't mention the removed features. However, I agree that some users may rely on this. All in all, it might be safer to merge this immediately to the master after we have forked off the release branch. |
- fail if config couldn't be loaded - remove duplicate api methods - remove undocumented XML loading feature - generate yaml conf in tests instead of xml conf - only load one config file instead of all xml or yaml files (flink-conf.yaml) - make globalconfiguration non-global and remove static SINGLETON - fix test cases - add test cases
|
Rebased to latest master. Merging this once Travis passes. |
- fail if config couldn't be loaded - remove duplicate api methods - remove undocumented XML loading feature - generate yaml conf in tests instead of xml conf - only load one config file instead of all xml or yaml files (flink-conf.yaml) - make globalconfiguration non-global and remove static SINGLETON - fix test cases - add test cases This closes apache#2123
This PR contains two commits. The first commit simply checks whether
loadConfiguration(..)has been called. The second commit adds additional tests and forbids malformed XML files.