-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ref #FLUME-3361 Added support for environment variable in ZooKeeper b… #326
base: trunk
Are you sure you want to change the base?
Conversation
…ased conf by setting -DpropertiesImplementation=org.apache.flume.node.EnvVarResolverProperties . This is consistent with the way environment variables are supported in local file based Flume agent conf.
… conf by setting -DpropertiesImplementation=org.apache.flume.node.EnvVarResolverProperties . This is consistent with the way environment variables are supported in local file based Flume agent conf. Also fixed code style issues.
Fixed code style issues. |
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.
Generally I'm okay with this, other than the point raised. What would the intention be for this though? To support non-properties file configurations? Where does this lead to?
properties.load(new StringReader(fileContent)); | ||
configMap = toMap(properties); | ||
Map<String, String> configMap = Collections.emptyMap(); | ||
if (configData != null && configData.length > 0) { |
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 else case previously set configMap = Collections.emptyMap(). We should retain this behaviour.
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.
Thanks for taking it up Tristan!
I have initialized configMap to emptyMap (Map<String, String> configMap = Collections.emptyMap();) so that an emptyMap (and not null) is returned even in case of a handled exception.
Tried to explain in detail below. Please let me know if I'm missing anything.
BEFORE:
Map m = null
if config does not exists
m = empty Map
return m
else
m = Map with config value //IO Exception is thrown
return m
NOW:
Map m = empty Map (so that I don't end up returning null in case of exceptions below)
if config exists
m = Map with config value
catch new possible exceptions other than IO
return m //so returning empty map like before (retaining the behavior)
//if Config does not exist or if an exception occurs that is handled
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.
Hi Tristan, please let me know if you are waiting for anything else from me.
Hey Tristan, Thanks |
Added support for environment variable in ZooKeeper based conf by setting
-DpropertiesImplementation=org.apache.flume.node.EnvVarResolverProperties . This is consistent with the way environment variables are supported in local file based Flume agent conf.