tgravescs commented on pull request #34710: URL: https://github.com/apache/spark/pull/34710#issuecomment-982750489
> Some internal code, we add some plugin code in internal code and need this value. This is not a public api, it has been brought up before about allowing this to be public but did not go anywhere. My concern here is making changes that make it partially supported but not really and it would be easily broken as devs don't know that use case. That variable is only used in functions for cluster mode so its not obvious at all it needs to be set for client mode. That Client class is unfortunately kind of a mix of things, where many of the functions are more like utility functions used in multiple places. This submitApplication I see as similar, where its called from multiple places and returns the appId that can be used however caller wants. There are other functions in there that take an appId, which doesn't really match, why not just use the appId in the class, its again kind of more like utility function setup, or just changes overtime that made it this way. While I don't agree with supporting this as a public api like it seems you are being used, I'm fine with doing some cleanup here but would like to see it done fully. I don't think submitApplication needs to return the appId, but we add method to get it. Then the YarnClientSchedulerBackend calls submitApplication() and then after calling that, it calls client.getApplicationId to pass into the bindToYarn function. There should also be a comment on the Client.appId saying setting to ensure works for both cluster and client mode. I also think monitorApplication and getApplicationReport should be changed to not take the appId but use the one in Client class. If they are truly utility functions that take the appId they should be moved to the Client object, but I think both of those functions use other internal Client variables or functions. The createContainerLaunchContext change made here, one could argue we should be looking at the app response and make sure it matches the appId the Client thinks it has rather then blindly assuming its from this Client. I think it was safer before then it is now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
