[
https://issues.apache.org/jira/browse/YARN-5324?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15485752#comment-15485752
]
Subru Krishnan commented on YARN-5324:
--------------------------------------
Thanks [~curino] for the patch. I looked at it and have a few comments below.
Can we move *SubClusterIdInfo* --> federation.store.records.dao as it can
potentially be used elsewhere like in future Federation Admin REST APIs.
In *WeightedPolicyInfo*:
* Rename _routerWeights_ --> _routerPolicyWeights_.
* Rename _amrmWeights_ --> _AMRMPolicyWeights_
* Add Javadocs for the getter/setters of the weights as they are not very
inituitive.
* Why are we iterating through the weight maps for every get
(_getRouterWeights/getAmrmWeights_)? Either we should avoid this or do this
once at initialization.
* We should move the JSONJAXBContext and marshaller/unmarshaller as class
variables and initialize at declaration as they are expensive ops.
* Can we give some example for alpha values and their effect to provide more
context.
* To improve readability, can we split the if into two - for routerWeights &
amrmWeights respectively;
{code}
if(otherAMRMWeights != null && amrmWeights != null &&
otherRouterWeights != null && routerWeights != null) {
return CollectionUtils.isEqualCollection(otherAMRMWeights.entrySet(),
amrmWeights.entrySet()) && CollectionUtils.isEqualCollection(
(otherRouterWeights.entrySet()), routerWeights.entrySet());
{code}
* For Hashcode - {code} 31 * amrmWeights + routerWeights {code} should be a
better option.
In *BaseWeightedRouterPolicy*:
* Something seems off in the sentence formation of class Javadocs.
* _getRand()_ seems to be used only by *WeightedRandomRouterPolicy* so can be
moved as a local variable as is done in *UniformRandomRouterPolicy*.
* Shouldn't {code} if (policyInfo != null &&
policyInfo.equals(newPolicyInfo)) { {code} be {code} if (policyInfo == null ||
policyInfo.equals(newPolicyInfo)) { {code}
* Why are we not checking _amrmWeights_ in:
{code}
if (newWeights == null || newWeights.size() < 1) {
{code}
* I think it'll be good if we move all the validations to either a validator
or a separate validate method.
* We should have the check for active sub-clusters here as now every policy
repeats it.
* I feel we should define the {{protected SubClusterId
selectSubCluster(Map<SubClusterId, SubClusterInfo> activeSubclusters,
Map<SubClusterId, Float> routerPolicyWeights)}} in the base class and
implementing policies can override it accordingly.
In *LoadBasedRouterPolicy*:
* We should invoke _getAvailableMemory_ only if weight is 1.
* Do we have to do this for every invocation of _getHomeSubcluster_. Seems
like it but just want to make sure?
In *OrderedRouterPolicy*:
* Typo in Javadoc; _Heights_ should be _Highest_.
* I feel it should round robin between the active sub-clusters as current
policy will pick the same sub-cluster every time (assuming that entire
sub-cluster downtime is rare, more so with RM HA). This feels more like a
*PriorityRouterPolicy*.
In *WeightedRandomRouterPolicy*:
* {code} if (getPolicyInfo() == null) { {code} isn't the check redundant?
* Can we add some code comments to clarify how exactly the selection is done.
*
{code}
chosen = id;
if (lookupValue <= 0) {
break;
}
{code}
should be
{code}
if (lookupValue <= 0) {
return id;
}
{code}
In *MockPolicyManager*:
* Might be better to return "default" rather than null in _getQueue_?
Common across tests:
* Am I missing something; All the tests call it out but I don't see scenarios
covering multiple invocations or clusters going inactive?
* We should have a {{BaseRouterPolicyTest}} and move the tests there and
override only the policy context in individual policy test. Refer to
{{FederationStateStoreBaseTest}}.
* How is
*FederationPoliciesTestUtil::createResourceRequests/createResourceRequest*
used? Why can't we use *ResourceRequest::newInstance* for the latter?
* Nit: variables can be declared outside the loop in _setUp_
In *TestLoadBasedRouterPolicy*:
* Typo in class Javadoc: _weighiting_ --> _weighting_
> Stateless router policies implementation
> ----------------------------------------
>
> Key: YARN-5324
> URL: https://issues.apache.org/jira/browse/YARN-5324
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Affects Versions: YARN-2915
> Reporter: Carlo Curino
> Assignee: Carlo Curino
> Attachments: YARN-5324-YARN-2915.06.patch,
> YARN-5324-YARN-2915.07.patch, YARN-5324-YARN-2915.08.patch,
> YARN-5324-YARN-2915.09.patch, YARN-5324.01.patch, YARN-5324.02.patch,
> YARN-5324.03.patch, YARN-5324.04.patch, YARN-5324.05.patch
>
>
> These are policies at the Router that do not require maintaing state across
> choices (e.g., weighted random).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
