[FLINK-2935] [scala-shell] Allow Scala shell to connect Flink cluster on YARN #1500
Conversation
| bin/start-scala-shell.sh yarn | ||
| ~~~ | ||
|
|
||
| The shell reads the connection information of the deployed Flink cluster from a `.yarn-properties` file in temporary directory. If there is no deployed Flink cluster on YARN, the shell prints error message. |
There was a problem hiding this comment.
prints an error message.
There was a problem hiding this comment.
from the .yarn-properties-USER file_, which is created in the configured yarn.properties-file.location directory or the temporary directory_.
|
Thanks for your work @chiwanpark. On the whole your changes look good. I had some minor comments. I also think that this won't work if the YARN cluster was started with HA and the first master has died. The reason is that the |
|
I have to correct myself. This should not be an issue if you start a Yarn cluster in HA and then use However, it should be a problem here. The |
chiwanpark
force-pushed
the
FLINK-2935
branch
from
4e57f68
to
e236d3b
Compare
|
@tillrohrmann Thanks for review! I have addressed your comments. |
|
About the type of number of containers for TaskManagers, it should be optional because the user can the Scala Shell without specifying the number when the user wants to connect already deployed YARN cluster. ( The number is required in only case of deploying YARN cluster for the shell. |
|
@tillrohrmann do you have any comments on @chiwanpark's updates? |
|
|
||
| File yarnPropertiesFile = new File(propertiesFileLocation + File.separator + CliFrontend.YARN_PROPERTIES_FILE + currentUser); | ||
| // file that we write into the conf/ dir containing the jobManager address and the dop. | ||
| File yarnPropertiesFile = new File(getYarnPropertiesLocation(yarnCluster.getFlinkConfiguration())); |
There was a problem hiding this comment.
The yarnPropertiesFile path construction is also done in CliFrontend:173. This should also be changed to use the new getYarnPropertiesLocation method. Maybe moving this method to CliFrontend makes sense.
|
Changes look good. I had only one minor comment left. After addressing this comment, I think we can merge the PR. |
|
@tillrohrmann Thanks for review. I'll address the comment in today. |
chiwanpark
force-pushed
the
FLINK-2935
branch
from
e236d3b
to
bffb344
Compare
|
I've addressed @tillrohrmann comment. After running CI, we can merge this. |
|
Failing test seems not related to this PR. |
… on YARN - Remove duplicated parseHostPortAddress method (Move it to ClientUtils class) - Refactor FlinkShell class
|
Hi all, I would like to merge this. If there is no objection and travis passes, I'll merge this to master in few days. |
|
I just found PR #1741 which contains a lot of changes of YARN classes. I'll wait until merging them. |
|
Thanks @chiwanpark! As far as I can see, there are no overlapping classes between the two pull requests. |
|
Oh, thanks for pointing it @mxm. Then I'll merge this. |
… on YARN - Remove duplicated parseHostPortAddress method (Move it to ClientUtils class) - Refactor FlinkShell class This closes apache#1500.
… on YARN - Remove duplicated parseHostPortAddress method (Move it to ClientUtils class) - Refactor FlinkShell class This closes apache#1500.
… on YARN - Remove duplicated parseHostPortAddress method (Move it to ClientUtils class) - Refactor FlinkShell class This closes apache#1500.
… on YARN - Remove duplicated parseHostPortAddress method (Move it to ClientUtils class) - Refactor FlinkShell class This closes apache#1500.
… on YARN - Remove duplicated parseHostPortAddress method (Move it to ClientUtils class) - Refactor FlinkShell class This closes apache#1500.
Please check a JIRA issue related to this PR. This PR is tested with Hadoop YARN 2.6.2. It works on Scala 2.10 and 2.11 both.
Because there is duplicated method
parseHostPortAddressinCliFrontendandRemoteExecutorclasses, I moved the method to a new classClientUtils. This PR contains also some refactoring ofFlinkShellclass.Any comments are welcome.