[ https://issues.apache.org/jira/browse/YARN-8048?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16429280#comment-16429280 ]
Gour Saha commented on YARN-8048: --------------------------------- [~rohithsharma] thank you for the patch. Sorry for the late review. This is a very interesting and helpful feature and the patch looks great. Few minor comments which can be taken care of in an addendum patch of maybe another sub-task under the parent (if they make sense to you) - 1. SystemServiceManagerImpl.java {code:java} if (!services.add(service)) { int count = ignoredUserServices.containsKey(userName) ? ignoredUserServices.get(userName) : 0; ignoredUserServices.put(userName, count + 1); LOG.warn( "Ignoring service {} for the user {} as it is already present," + " filename = {}", service.getName(), userName, filename); } LOG.info("Added service {} for the user {}, filename = {}", service.getName(), userName, filename); {code} I think the info log will get printed here every time the warn log is also printed inside the if block. Should the info log go in the else block? 2. Should we rename TestSystemServiceImpl.java to TestSystemServiceManagerImpl.java? 3. TestSystemServiceImpl {code:java} Assert.assertTrue( "Service name doesn't exist in expected " + "userService " + serviceNames, serviceNames.contains(next.getName())); {code} Should we combine these 2 strings into 1 -> "Service name doesn't exist in expected " + "userService " 4. Do we need to add documentation about "yarn.service.system-service.dir” anywhere? 5. Under hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/test/resources/ should we rename the dir “users" to "system-services" to better reflect what these files/tests are for (and change resourcePath in TestSystemServiceImpl.java accordingly) 6. For an additional test to see if a dir is skipped, do you want to add a directory named “bad" (probably needs a dummy file under it otherwise github will not allow you to commit an empty dir) under the path hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/test/resources//users/sync/user1/? > Support auto-spawning of admin configured services during bootstrap of > rm/apiserver > ----------------------------------------------------------------------------------- > > Key: YARN-8048 > URL: https://issues.apache.org/jira/browse/YARN-8048 > Project: Hadoop YARN > Issue Type: Sub-task > Reporter: Rohith Sharma K S > Assignee: Rohith Sharma K S > Priority: Major > Fix For: 3.2.0 > > Attachments: YARN-8048.001.patch, YARN-8048.002.patch, > YARN-8048.003.patch, YARN-8048.004.patch, YARN-8048.005.patch, > YARN-8048.006.patch > > > Goal is to support auto-spawning of admin configured services during > bootstrap of resourcemanager/apiserver. > *Requirement:* Some of the services might required to be consumed by yarn > itself ex: Hbase for atsv2. Instead of depending on user installed HBase or > sometimes user may not required to install HBase at all, in such conditions > running HBase app on YARN will help for ATSv2. > Before YARN cluster is started, admin configure these services spec and place > it in common location in HDFS. At the time of RM/apiServer bootstrap, these > services will be submitted. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org