Thanks Carlo Curino for the patch. I looked at it, please find my comments below.
- It looks like you have the intellij formatting set rather than the Hadoop standard. For e.g. : Unnecessary change to imports in FederationStateStoreFacade. I feel this is the root cause for the checkstyle warnings too.
- For every new config, you need to add corresponding default in yarn-default.xml or exclusions in TestYarnConfigurationFields. I prefer the latter for these configs.
- I don't think we need DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS (can we avoid FEDERATION_POLICY_MANAGER_PARAMS itself) in the interest of trying to limit the configuration explosion.
String defaultPolicyParamString = conf.get(YarnConfiguration.DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS, YarnConfiguration.DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS);
String defaultPolicyParamString = conf.get(YarnConfiguration.FEDERATION_POLICY_MANAGER_PARAMS, YarnConfiguration.DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS);
- Please use Charset.defaultCharset() instead of hard-coding (also existing usages like in WeightedPolicyInfo.
- I feel it's better if we add the the fallbackPolicyManager to the cache during initialization so that we can avoid costly fallbackPolicyManager::getRouterPolicy in every invocation of getHomeSubcluster.
- We should have a warning log with context that we were not able to retrieve the policy configuration for the input queue.
- The if clause can be rephrased as
cachedConfs.containsKey(queue) && !cachedConfs.get(queue).equals(configuration)
- We should specify for what queue, we got null from StateStore and possibly log it.
- Overall the coverage is nice! The only feedback is it took me quite some time to figure out how the configuration was getting updated in testConfigurationUpdate. IIUC, it's done implicitly through FederationPoliciesTestUtil::initFacade? If so, can we do it explicitly as that will help considerably in readability and moreover none of the other tests use it and might even be a undesirable side-effect.
- Nit: can we rename testgetHomeSubcluster to testGetHomeSubcluster and move it up to the first test.