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-5488] stop YarnClient before exception is thrown #4022
Conversation
@@ -392,6 +392,7 @@ protected YarnClient getYarnClient() { | |||
@Override | |||
public YarnClusterClient retrieve(String applicationID) { | |||
|
|||
final YarnClient yarnClient = getYarnClient(); | |||
try { | |||
// check if required Hadoop environment variables are set. If not, warn user | |||
if (System.getenv("HADOOP_CONF_DIR") == null && |
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.
does getYarnClient()
fail if this condition is fulfilled? If so we should put this if block before the call to getYarnClient()
.
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.
It's a good point, I will put getYarnClient()
in the try
block
@@ -420,6 +421,9 @@ public YarnClusterClient retrieve(String applicationID) { | |||
|
|||
return createYarnClusterClient(this, yarnClient, appReport, flinkConfiguration, false); | |||
} catch (Exception e) { | |||
if(null != yarnClient) { |
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.
missing space after if.
@@ -146,7 +146,9 @@ public ApplicationStatus getApplicationStatus() { | |||
|
|||
@Override | |||
public void finalizeCluster() { | |||
// Do nothing |
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.
Please revert the changes here. This file is part of another big initiative (referred to as FLIP-6); we shouldn't touch it unless necessary. (basically, I don't know about the yarn-client life-cycle here so let's be conservative.)
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.
I have reverted the changes and fixed another problem as Ted Yu metioned in FLINK-5488, thanks @zentol
merging. |
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.
General
Documentation
Tests & Build
mvn clean verify
has been executed successfully locally or a Travis build has passed