[
https://issues.apache.org/jira/browse/YARN-5325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15536931#comment-15536931
]
Subru Krishnan commented on YARN-5325:
--------------------------------------
Thanks [~curino] for working on this. I looked at the patch, please find my
comments below.
*General*:
* We should rename _Base*_ classes to _Abstract*_ as that's the naming
convention followed in YARN. For e.g. : {{AbstractYarnScheduler}}.
* There are a few typos in Javadocs especially
{{LocalityMulticastAMRMProxyPolicy}}.
* The {{SubClusterResolver}} should not throw _YarnException_ but instead
fallback to _home sub-cluster_ ala DEFAULT_RACK for the {{RackResolver}}.
*LocalityMulticastAMRMProxyPolicy*:
* Creating {{AllocationBookkeeper}} for each split request seems costly.
Would it be to possible to pre-initialize and reuse?
* Use _Math.round_ instead of _(int) Math.ceil_ in _splitIndividualAny_.
* I feel in {{AllocationBookkeeper}} initialization, we should loop on
weights as _num(configured clusters) << num(active clusters)_.
* We should collapse the three for loops in {{AllocationBookkeeper}}
initialization into a single one and the above suggestion should help doing
that.
* I don't see the purpose of using AtomicLongs and TreeMaps in
{{AllocationBookkeeper}} as we seem to be the incurring the overhead without
using the additional attributes.
*Tests* (generally a good job at coverage :)):
* The scenario that's missing is multiple/subsequent invocations with
combination of state changing like policy weights, active sub-clusters,
headroom. Can you please add.
* We should throw rather than ignore exceptions as that has useful
information for debugging in case of test failures.
* I feel we should avoid _System.outs_.
* Can we directly use {{ResourceRequest::newInstance}} instead of introducing
a local custom {{FederationPoliciesTestUtil::createResourceRequest}}?
* {{TestLocalityMulticastAMRMProxyFederationPolicy ::testStressPolicy}}
doesn't seem to be doing any assertions?
*Questions*:
* Currently the implementation is not thread-safe. I assume that's a
conscious decision given that {{AMRMProxyService}} creates a per-application
pipeline?
* I am still trying to grok the correctness and invariance satisfaction for
_getHeadroomWeighting_. At first pass, looks good. I'll revert in case I have
any concerns.
* Similarly I am still in the process of verifying the splitting of
_numContainers_ in {{TestLocalityMulticastAMRMProxyFederationPolicy}}. If
possible, can you add code comments.
> Stateless ARMRMProxy policies implementation
> --------------------------------------------
>
> Key: YARN-5325
> URL: https://issues.apache.org/jira/browse/YARN-5325
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Affects Versions: YARN-2915
> Reporter: Carlo Curino
> Assignee: Carlo Curino
> Attachments: YARN-5325-YARN-2915.05.patch,
> YARN-5325-YARN-2915.06.patch, YARN-5325-YARN-2915.07.patch,
> YARN-5325.01.patch, YARN-5325.02.patch, YARN-5325.03.patch, YARN-5325.04.patch
>
>
> This JIRA tracks policies in the AMRMProxy that decide how to forward
> ResourceRequests, without maintaining substantial state across decissions
> (e.g., broadcast).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
