[
https://issues.apache.org/jira/browse/YARN-1305?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13800355#comment-13800355
]
Bikas Saha commented on YARN-1305:
----------------------------------
Thanks for your patience through numerous review iterations.
Why is verifyConf() setting values in the conf? Its fine to do it as long as we
rename the method to do something like verifyAndSetConfValue(). In RMHAService
we can call verifyAndSetConfiguration() and remove the redundant
setAllRpcAddresses.
{code}
+ private static void verifyConfValue(String prefix, Configuration conf) {
+ String confKey = null;
+ String confValue = null;
+ try {
+ confKey = getConfKeyForRMInstance(prefix, conf);
+ confValue = getConfValueForRMInstance(prefix, conf);
+ conf.set(prefix, confValue); <=========================== Here
{code}
All the @visibleForTesting methods can be made package private instead of
public.
The body of this test seems to have been removed? Is this a bad diff or am I
misreading the patch?
{code}
@Test
public void testGetRMId() throws Exception {
assertEquals("Does not honor " + YarnConfiguration.RM_HA_ID,
- RM1_NODE_ID, HAUtil.getRMHAId(conf));
- conf = new YarnConfiguration();
- try {
- HAUtil.getRMHAId(conf);
- fail("getRMHAId() fails to throw an exception when RM_HA_ID is not set");
- } catch (YarnRuntimeException yre) {
- // do nothing
- }
+ RM1_NODE_ID.trim(), HAUtil.getRMHAId(conf));
}
{code}
Minor nits
Can we simply set the trimmed value in the conf during verifyAndSet() instead
of trimming it every time?
{code}
public static String getRMHAId(Configuration conf) {
- String rmId = conf.get(YarnConfiguration.RM_HA_ID);
- if (rmId == null) {
- throwBadConfigurationException(YarnConfiguration.RM_HA_ID +
- " needs to be set in a HA configuration");
- }
- return rmId;
+ return conf.getTrimmed(YarnConfiguration.RM_HA_ID);
{code}
Can we create RM1_ADDRESS_UNTRIMMED for use only in the setup() method and
allow the remaining test methods so simply use RM1_ADDRESS. I think this will
make the test cleaner and easier to understand and maintain.
{code}
@@ -73,7 +74,71 @@ public void testSetGetRpcAddresses() throws Exception {
HAUtil.setAllRpcAddresses(conf);
for (String confKey : HAUtil.RPC_ADDRESS_CONF_KEYS) {
assertEquals("RPC address not set for " + confKey,
- RM1_ADDRESS, conf.get(confKey));
+ RM1_ADDRESS.trim(), conf.get(confKey));
{code}
> RMHAProtocolService#serviceInit should handle HAUtil's
> IllegalArgumentException
> -------------------------------------------------------------------------------
>
> Key: YARN-1305
> URL: https://issues.apache.org/jira/browse/YARN-1305
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: resourcemanager
> Affects Versions: 2.2.1
> Reporter: Tsuyoshi OZAWA
> Assignee: Tsuyoshi OZAWA
> Labels: ha
> Attachments: YARN-1305.1.patch, YARN-1305.2.patch, YARN-1305.3.patch,
> YARN-1305.4.patch, YARN-1305.5.patch, YARN-1305.6.patch, YARN-1305.7.patch
>
>
> When yarn.resourcemanager.ha.enabled is true, RMHAProtocolService#serviceInit
> calls HAUtil.setAllRpcAddresses. If the configuration values are null, it
> just throws IllegalArgumentException.
> It's messy to analyse which keys are null, so we should handle it and log the
> name of keys which are null.
> A current log dump is as follows:
> {code}
> 2013-10-15 06:24:53,431 INFO
> org.apache.hadoop.yarn.server.resourcemanager.ResourceManager: registered
> UNIX signal handlers for [TERM, HUP, INT]
> 2013-10-15 06:24:54,203 INFO org.apache.hadoop.service.AbstractService:
> Service RMHAProtocolService failed in state INITED; cause:
> java.lang.IllegalArgumentException: Property value must not be null
> java.lang.IllegalArgumentException: Property value must not be null
> at
> com.google.common.base.Preconditions.checkArgument(Preconditions.java:88)
> at org.apache.hadoop.conf.Configuration.set(Configuration.java:816)
> at org.apache.hadoop.conf.Configuration.set(Configuration.java:798)
> at org.apache.hadoop.yarn.conf.HAUtil.setConfValue(HAUtil.java:100)
> at
> org.apache.hadoop.yarn.conf.HAUtil.setAllRpcAddresses(HAUtil.java:105)
> at
> org.apache.hadoop.yarn.server.resourcemanager.RMHAProtocolService.serviceInit(RMHAProtocolService.java:60)
> at
> org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
> at
> org.apache.hadoop.service.CompositeService.serviceInit(CompositeService.java:108)
> at
> org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.serviceInit(ResourceManager.java:187)
> at
> org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
> at
> org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.main(ResourceManager.java:940)
> {code}
--
This message was sent by Atlassian JIRA
(v6.1#6144)
