[
https://issues.apache.org/jira/browse/YARN-10201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17086515#comment-17086515
]
Íñigo Goiri commented on YARN-10201:
------------------------------------
Thanks [~youchen] for the updated patch.
A few comments:
* Can we extend the javadocs in EnhancedHeadroom, in particular extending the
main description to put this in the general context. Then it would be nice to
have javadocs for each of the methods and putting examples of values.
* Modifying DEFAULT_FEDERATION_AMRMPROXY_SUBCLUSTER_TIMEOUT I think is out of
the scope of this JIRA.
* EnhancedHeadroomPBImpl#102,118 no need for the first parenthesis.
* Not sure we should be changing the clock AMRMClientRelayer. Probably a
separate JIRA.
* Add javadocs to the new methods in AMRMClientRelayer. I would also extract
the values like {{this.remotePendingAsks.get(key)}}.
* Avoid the empty line change in AMRMClientRelayer#584.
* ContainerAsksBalancer#LOGdebug, let's use the logger format with {}.
* Extend the import for LocalityMulticastAMRMProxyPolicy#YarnConfiguration.*.
* Don't use javadoc comment style inside
LocalityMulticastAMRMProxyPolicy#pickSubClusterIdForMaxLoadSC(). That could
potentially go into the method javadoc actually. getSubClusterLoad() should
also have a javadoc.
* Javadoc with example for SubClusterId#getShortId(), it would be good if it
had examples.
* For testLoadbasedSubClusterReroute, let's add a general javadoc explaining
the idea of the test.
* In the test let's do a static import for assertTrue and assertEquals,
actually there are a few places where we do assertTrue(X.equals(Y)), let's do
assertEquals.
* In the test, let's use 1.0 / 16.0 instead of 0.0625, etc.
* In FederationInterceptor#policyInterpreterLock add a one-line javadoc
specifying what we are locking.
* Overall, I think we should go some more tests a little more specific, ideas
that come to mind are the whole protobuf. ContainerAsksBalancer, and the code
path that leads to routeNodeRequestIfNeeded.
* testLoadbasedSubClusterReroute() is pretty complete but I feel we could have
a separate class for it and have smaller and simpler tests there too.
> Make AMRMProxyPolicy aware of SC load
> -------------------------------------
>
> Key: YARN-10201
> URL: https://issues.apache.org/jira/browse/YARN-10201
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: amrmproxy
> Reporter: Young Chen
> Assignee: Young Chen
> Priority: Major
> Attachments: YARN-10201.v0.patch, YARN-10201.v1.patch,
> YARN-10201.v2.patch, YARN-10201.v3.patch, YARN-10201.v4.patch,
> YARN-10201.v5.patch
>
>
> LocalityMulticastAMRMProxyPolicy is currently unaware of SC load when
> splitting resource requests. We propose changes to the policy so that it
> receives feedback from SCs and can load balance requests across the federated
> cluster.
--
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
