-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-6581] [cli] Correct dynamic property parsing for YARN cli #3903
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
[FLINK-6581] [cli] Correct dynamic property parsing for YARN cli #3903
Conversation
The YARN cli will now split the dynamic propertie at the first occurrence of the = sign instead of splitting it at every = sign. That way we support dynamic properties of the form -yDenv.java.opts="-DappName=foobar".
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.
Had one question, otherwise +1.
String key = propLine.substring(0, firstEquals).trim(); | ||
String value = propLine.substring(firstEquals + 1, propLine.length()).trim(); | ||
|
||
if (!key.isEmpty() && !value.isEmpty()) { |
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.
We didn't reject empty values before. Not sure if there's a valid use-case for that though, maybe overriding/clearing some variable?
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.
True, in order to keep in sync with the old behaviour, I'll remove the check whether a value is empty.
Thanks for the review @zentol. I'll address your comment and then merge the PR once Travis gives green light. |
+1 |
Thanks for turning this around quickly @tillrohrmann We are experiencing this issue with AWS EMR. Is it possible to patch the current 1.2.0 version available via EMR? |
@johnboy14 I can merge this PR also into the |
When's the next bug fix release scheduled for @tillrohrmann ? |
This is not decided yet. Usually we do it depending on the urgency of the fixes and the time since the last bug fix release. Currently, we're working on the |
At this time I am looking a workaround. Is there anyway to override the log4j.properties file for each application deployed to EMR? |
The YARN cli will now split the dynamic propertie at the first occurrence of
the = sign instead of splitting it at every = sign. That way we support dynamic
properties of the form -yDenv.java.opts="-DappName=foobar".