MYRIAD-184 RM Ports are Hardcoded in NMExecutorCLGenImpl.java #61
Conversation
|
Is this a good idea? Seems like we're adding more config to Myriad maybe we should just throw a runtime exception? |
|
As in assert that the port be the default port as we are going now, where we throw an exception should there be no response at the given default port address? |
|
8088 is yarns default HTTP port and 8090 is the HTTPS port. If neither are in the yarn-site.xml they'll revert to those. The original check in code was for sanity in case the yarn configuration didn't have those values and returned a null. Adding additional config here could lead to a conflict between yarn and Myriad's perception of the correct port. The assert would be do we get a value (not null or empty). |
|
A good question would be how are people currently deploying their myriad servers, if they are using some kind of configuration management tool, this makes live much easier if you want to change the port, as you can just create a template file where you can then override the yarn http port. If a person is using everything the way using the default configs from both yarn and myriad, then the config is already set as I had done. That said if you think its best to go the other way then how would you recommend making such an assertion, is there a yarn endpoint we can call to see if that port is the default, is it even worth making such a request if it has to be done over http. |
| @@ -164,13 +164,13 @@ public String getConfigurationUrl() { | |||
| if (httpPolicy != null && httpPolicy.equals(TaskFactory.YARN_HTTP_POLICY_HTTPS_ONLY)) { | |||
| String address = conf.get(TaskFactory.YARN_RESOURCEMANAGER_WEBAPP_HTTPS_ADDRESS); | |||
| if (address == null || address.isEmpty()) { | |||
| address = conf.get(TaskFactory.YARN_RESOURCEMANAGER_HOSTNAME) + ":8090"; | |||
| address = conf.get(TaskFactory.YARN_RESOURCEMANAGER_HOSTNAME) + cfg.getYarnResourceManagerPortHTTPS(); | |||
| } | |||
| return "https://" + address + "/conf"; | |||
There was a problem hiding this comment.
Basically Lines 166-168 were a last ditch effort to get the HTTPS port if it didn't exist in the YarnConfiguration object, after learning more it turns out this code is only hit if yarn.resourcemanager.webapp.https.address does not exist, however it defaults to yarn.resource.manager.hostnam:8090. So we can safely delete that code, making the additional configuration cruft unnecessary.
|
This makes sense to render the port configurable and implement the getter with Optional.of().or("8088") like I did in the MYRIAD-198 pull request. I think it would also be good to include a plugin for a service discovery tool like mesos-dns or consul. I especially like the latter in that consul supports the SRV protocol, meaning the dns abstracts away both the IP address and the port. |
|
I agree with @DarinJ. I think it is not necessary to enter the configuration because we cover the use cases of no configuration with the default case and use cases of configuration with: yarn.resourcemanager.webapp.https.address |
Moved port definitions to config file.