[
https://issues.apache.org/jira/browse/YARN-9761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16921602#comment-16921602
]
Jonathan Hung edited comment on YARN-9761 at 9/3/19 5:45 PM:
-------------------------------------------------------------
Thanks for the patch [~pralabhkumar], I have a few more comments on the latest
004 patch:
* I agree with [~adam.antal] on the new package. Perhaps
org/apache/hadoop/yarn/server/resourcemanager/preprocessor. We can move
ContextProcessor, [NodeLabel|TagAdd|Queue]Processor, and
SubmissionContextPreProcessor there.
* ContextProcessor.java:
** Can we remove the spaces before the periods in the class comment ("This is
the interface providing functionality to process application submission context
."), and the {{void process}} javadoc? And the space before "It"?
** Can we remove the colons after the params in javadoc? e.g. "host : Address
of the host..." -> "host Address of the host..."
** InterfaceStability import is using the wrong package, should be
{{org.apache.hadoop.classification}}
* SubmissionContextPreProcessor.java: for the IOUtils.cleanupWithLogger, I
think we can just combine the two into one line i.e.
{{IOUtils.cleanupWithLogger(LOG, reader, fileInputStream);}}
* TestClientRMService.java:
** For testSubmissionContextWithAbsentTAG, I think we should just either
inline this test with testAppSubmitWithSubmissionPreProcessor, or refactor all
of testAppSubmitWithSubmissionPreProcessor, i.e. have some helper function
which takes a host, and expected tag, nodelabel, and queue name and verifies
submitted app matches these expected entities.
** I think testApplicationSubmitContextProcess belongs in a separate test
class. Let's put it in the new package
(org/apache/hadoop/yarn/server/resourcemanager/preprocessor) under something
like TestContextProcessor. Let's put the NodeLabel/Queue/TagAdd processors in
separate test cases too.
** In testApplicationSubmitContextProcess, we don't need to create an entire
applicationSubmissionContext instance, let's mock this out.
** Space between {{()}} and { in {{testApplicationSubmitContextProcess(){}}
was (Author: jhung):
Thanks for the patch [~pralabhkumar], I have a few more comments on the latest
004 patch:
* I agree with [~adam.antal] on the new package. Perhaps
org/apache/hadoop/yarn/server/resourcemanager/preprocessor. We can move
ContextProcessor, [NodeLabel|TagAdd|Queue]Processor, and
SubmissionContextPreProcessor there.
* ContextProcessor.java:
** Can we remove the spaces before the periods in the class comment ("This is
the interface providing functionality to process application submission context
."), and the {{void process}} javadoc? And the space before "It"?
** Can we remove the colons after the params in javadoc? e.g. "host : Address
of the host..." -> "host Address of the host..."
** InterfaceStability import is using the wrong package, should be
{{org.apache.hadoop.classification}}
* SubmissionContextPreProcessor.java: for the IOUtils.cleanupWithLogger, I
think we can just combine the two into one line i.e.
{{IOUtils.cleanupWithLogger(LOG, reader, fileInputStream);}}
* TestClientRMService.java:
** For testSubmissionContextWithAbsentTAG, I think we should just either
inline this test with testAppSubmitWithSubmissionPreProcessor, or refactor all
of testAppSubmitWithSubmissionPreProcessor, i.e. have some helper function
which takes a host, and expected tag, nodelabel, and queue name and verifies
submitted app matches these expected entities.
** I think testApplicationSubmitContextProcess belongs in a separate test
class. Let's put it in the new package
(org/apache/hadoop/yarn/server/resourcemanager/preprocessor) under something
like TestContextProcessor. Let's put the NodeLabel/Queue/TagAdd processors in
separate test cases too.
** In testApplicationSubmitContextProcess, we don't need to create an entire
applicationSubmissionContext instance, let's mock this out.
** Space between {{()}} and {{{}} in
{{testApplicationSubmitContextProcess(){}}
> 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, YARN-9761.04.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: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
