[
https://issues.apache.org/jira/browse/YARN-9761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16920785#comment-16920785
]
Adam Antal commented on YARN-9761:
----------------------------------
Thanks for the patch. This feature is really a good idea.
- In {{SubmissionContextPreProcessor$refresh}} 's big try-catch-finally block:
If an exception happens during the finally part e.g. when the reader or
fileInputStream is closed an exception is thrown, if there was already an
exception in the try block, it would be omitted. Please use
IOUtils.cleanupWithLogger() to make sure that exception handling is not doing
stuff like that.
- Majority of the RM tests have failed due to the error
"java.lang.OutOfMemoryError: unable to create new native thread". Which makes
me wonder that somehow we may leak threads. Let's wait until the next patch to
see if it's a one-time problem, or we actually forget to add some stronger
shutdown logic than this.
- When the program terminates there's no need to wait for the refresh task of
{{SubmissionContextPreProcessor}}, because it won't be used in the future. So I
think you can use this.executorService.shutdownNow() instead of
this.executorService.shutdown() in {{SubmissionContextPreProcessor$stop}}.
- In {{TestClientRMService}}: Could you please add a line where not all the
three of the preprocessor rule is provided?
- Could you please also move the "mytag:foo" to a static final string class
variable in {{TestClientRMService$testAppSubmitWithSubmissionPreProcessor}}?
One major suggestion (free to discuss): I'd rather move the
{{ContextProcessor}} and its subclasses out of
{{SubmissionContextPreProcessor}}, maybe into their own files? As far as I see
this feature is designed to be possible to add more of those preprocessor
later. I think it would greatly improve the maintainability as well to move
these to separate classes, not to speak of preprocessors ({{ContextProcessor}})
are currently not unit tested by your patch, and it would be easier to do this
if they were in separate classes. I'd also consider to create a whole new
package for this, but it might not worth the effort. Please express your
opinion on this.
> Allow overriding application submissions based on server side configs
> ---------------------------------------------------------------------
>
> Key: YARN-9761
> URL: https://issues.apache.org/jira/browse/YARN-9761
> Project: Hadoop YARN
> Issue Type: New Feature
> Reporter: Jonathan Hung
> Assignee: pralabhkumar
> Priority: Major
> Labels: release-blocker
> Attachments: YARN-9761.01.patch, YARN-9761.02.patch,
> YARN-9761.03.patch
>
>
> Create a preprocessor/interceptor which takes each app submitted to RM and
> overrides the submission context based on server side configs.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
