[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement

["Peter Bacsko (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17265947#comment-17265947 ] 

Peter Bacsko edited comment on YARN-10535 at 1/15/21, 11:41 AM:
----------------------------------------------------------------

Initial review:

1. Remove star imports: {{import java.util.*;}}
 2. Remove star imports: {{import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.*;}}
 3. Is this class used? {{import org.apache.hadoop.yarn.webapp.hamlet2.Hamlet;}}
 4. Nit: some exceptions ends with ".", some with "!", some with no punctuation at all. Let's have some consistency in this area.
 5. Nit: I'd replace "Unknown dynamic path lookup error" to sth like "Unknown queue path validation result"
 6. {{MappingRuleValidationContextImpl}}: I'd definitely call {{hasNext()}} before calling {{String staticPart = pointer.next();}} just to be 100% safe
 7. {{MappingRuleValidationContextImpl}}: {{staticPartParent}} is initially {{null}}. Any change of it remaining {{null}} after the {{while}} loop? If so, a null check is warranted.
 8. Nit: I'd rename "QueueCreationValidation" to "QueueCreationValidationResult" or "AutoQueueCreateValidationResult" to reflect that it is a result of something. Or simply "ValidationResult", because it's a nested enum.

I can't really comment on the validation logic because there a lot of string operations and hopefully the unit tests will do their job.

Talking about unit tests: {{TestMappingRuleValidationContextImpl}} was extended with some new cases, does that provide enough coverage? Have you run it through a coverage tool? EDIT: ok, the patch does not compile yet, so probably the answer is "no".

Also, many checkstyle issue, pls fix them if time permits (mostly indentation).


was (Author: pbacsko):
Initial review:

1. Remove star imports: {{import java.util.*;}}
 2. Remove star imports: {{import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.*;}}
 3. Is this class used? {{import org.apache.hadoop.yarn.webapp.hamlet2.Hamlet;}}
 4. Nit: some exceptions ends with ".", some with "!", some with no punctuation at all. Let's have some consistency in this area.
 5. Nit: I'd replace "Unknown dynamic path lookup error" to sth like "Unknown queue path validation result"
 6. {{MappingRuleValidationContextImpl}}: I'd definitely call {{hasNext()}} before calling {{String staticPart = pointer.next();}} just to be 100% safe
 7. {{MappingRuleValidationContextImpl}}: {{staticPartParent}} is initially {{null}}. Any change of it remaining {{null}} after the {{while}} loop? If so, a null check is warranted.
 8. Nit: I'd rename "QueueCreationValidation" to "QueueCreationValidationResult" or "AutoQueueCreateValidationResult" to reflect that it is a result of something. Or simply "ValidationResult", because it's a nested enum.

I can't really comment on the validation logic because there a lot of string operations and hopefully the unit tests do their job.

Talking about unit tests: {{TestMappingRuleValidationContextImpl}} was extended with some new cases, does that provide enough coverage? Have you run it through a coverage tool?

Also, many checkstyle issue, pls fix them if time permits (mostly indentation).

> Make queue placement in CapacityScheduler compliant with auto-queue-placement
> -----------------------------------------------------------------------------
>
>                 Key: YARN-10535
>                 URL: https://issues.apache.org/jira/browse/YARN-10535
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: capacity scheduler
>            Reporter: Wangda Tan
>            Assignee: Gergely Pollak
>            Priority: Major
>         Attachments: YARN-10535.001.patch
>
>
> Once YARN-10506 is done, we need to call the API from the queue placement policy to create queues. 



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org