[
https://issues.apache.org/jira/browse/YARN-10376?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17190602#comment-17190602
]
Szilard Nemeth commented on YARN-10376:
---------------------------------------
Hi [~shuzirra],
1. Nit: CSMappingPlacementRule#buildValidationContext can be private.
2. CSMappingPlacementRule#buildValidationContext: What's the point of passing
the two fields (immutableVariables, queueManager) to this method? As
buildValidationContext is an instance method as well, it can access these
fields directly.
3. Still in CSMappingPlacementRule#buildValidationContext: This error message
looks / sounds weird:
{code}
LOG.error("Error initializing placement variables unable to register" +
" '%default': " + e.getMessage());
{code}
4. Nit: Shouldn't we already have a constant for "%default" somewhere? I cannot
find it in the codebase.
5. CSMappingPlacementRule#initialize: It's better to refer to the class name in
LOG / exception strings with getClass().getSimpleName().
6. CSMappingPlacementRule#initialize: You can use parameterized logging instead
of String concatenation in the LOG.error(..) statement. This way you become
more consistent with the rest of the code, as some lines below this there's a
parameterized LOG.info call.
7. CSMappingPlacementRule#initialize: When you debug log the rules, I think
it's better to add one log statement like: "Printing mapping rules: ", then you
can have the forEach-debug print you already added to the code.
8. Code duplication in CSMappingPlacementRule#getSecondaryGroup with
UserGroupMappingPlacementRule#getSecondaryGroup: Can you extract a static
method and pass the QM instance and the groups instance? Would this make sense
or do you have different plans to handle this duplication?
9. Nit: CSMappingPlacementRule#getPlacementForApp: Comment in the beginning
looks weird with two spaces indentation from the 2nd line.
10. CSMappingPlacementRule#getPlacementForApp: I think it's better to add log
messages, at least for the cases where you return null, for better tracebility.
11. CSMappingPlacementRule#getPlacementForApp: In case REJECT, the info log has
the word 'action' twice: "fallback action action".
12. TestCSMappingPlacementRule: Extending Testcase and using @Test annotations
is unconventional, please fix.
13. TestCSMappingPlacementRule: There are several fields and methods that can
be private: userGroups, createQueueHierarchy, setupEngine, createApp,
assertReject, assertPlace, assertNull.
14. Please add messages to asserts in methods: assertReject, assertPlace and
assertNull.
15. There are two error logs defined as:
{code}
LOG.error("{} {}", message, e);
{code}
"Intellij complains with: "The formatted log message expects 2 arguments,
passed 1."
I guess this is because e is an instance of Exception, so I guess you should
use the error method that expects a String + an exception as a parameter like:
{code}
LOG.error(message, e);
{code}
16. Typos in TestCSMappingPlacementRule#testLegacyPlacementToExistingQueue:
"defaule", "charile"
17. Last statement of
TestCSMappingPlacementRule#testLegacyPlacementToManagedQueues can fit into one
single line.
18. Messages in TestCSMappingPlacementRule#testLegacyPlacementShortReference
could be easier to understand if the string prefix would be "Should be placed
to default: " instead of the current format without colon.
19. Typo in TestCSMappingPlacementRule#testRuleFallbackHandling: "Charile"
20. Typo in TestCSMappingPlacementRule#testConfigValidation: Variable name:
nonExistantStatic should be nonExistentStatic
21. I guess it could be more straightforward to split the testcase
TestCSMappingPlacementRule#testConfigValidation into 3 separate testcases.
Also, the catch-blocks are empty. Is this how they supposed to be?
> Create a class that covers the functionality of UserGroupMappingPlacementRule
> and AppNameMappingPlacementRule using the new mapping rules
> -----------------------------------------------------------------------------------------------------------------------------------------
>
> Key: YARN-10376
> URL: https://issues.apache.org/jira/browse/YARN-10376
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: yarn
> Reporter: Gergely Pollak
> Assignee: Gergely Pollak
> Priority: Major
> Attachments: YARN-10376.001.patch, YARN-10376.002.patch
>
>
> As per the design document attached to the umbrella Jira (YARN-10370), we
> need to replace the current PlacementRule classes with a new generic one,
> which can utilize the more generic MappingRule approach. This way we can even
> mix User and Application placement rules and define a custom order between
> all the rules, not only the rules within the same category (user or
> applcation placement rules)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
