> On Oct. 3, 2014, 1:23 a.m., jun aoki wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/util/StackExtensionHelper.java, > > line 431 > > <https://reviews.apache.org/r/26057/diff/1/?file=705685#file705685line431> > > > > Thank you Sid for your comment. > > Correct me if I don't get it. > > > > AmbariMetaInfo#getConfigurationInformation calls a set of > > StackExtentionHelper's methods in order of fillInfo(), > > getAllAvailableStacks() and getAllApplicableServices() > > > > And you are suggesting > > 1. Delete existing actionMetadata#addServceCheckAction (line 488) from > > getAllApplicableServices() > > ``` > > actionMetadata.addServiceCheckAction(serviceInfo.getName()) > > ``` > > 2. Not add another actionMetadata defined in the current patch > > 3. Add actionMetadata in populateServicesForStack() because > > fillInfo() calls getStackInfo() which calls populateServicesForStack() > > > > The logic that will be added to populateServicesForStack is same > > 1. Look for parents, > > 2. if parents do not exist loop the given stackInfo's services, > > 3. If parents exist, loop with the parent's services. > > > > > > can you also take a look my comment in the JIRA? > > > > > > https://issues.apache.org/jira/browse/AMBARI-7442?focusedCommentId=14157221&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14157221 > > jun aoki wrote: > This is more of a response to Sid's comment above. > > Sid Wagle wrote: > 1. Delete existing actionMetadata#addServceCheckAction (line 488) from > getAllApplicableServices() > Yes, absolutely correct. > > 2. Not add another actionMetadata defined in the current patch > Yes to that as well. > > 3. Add actionMetadata in populateServicesForStack() > Correct, here as well, why do you need to check parent stack == null? The > actions are applicable nonetheless, so you just add it as a valid action. > If you want to disable a certain action in the inherticance tree, this > should be expressed explicitly with flag. > > jun aoki wrote: > Sid, thank you for clarifying. > > Bad news, > I did this way as directed by you. > https://reviews.apache.org/r/26334/ (another reviewboard only for sharing > the bad code with you.) > > Then I get a NullPointerException on stackParentsMap since > stackParentsMap is set at the end of fillInfo(). (see my comment on line 544 > of the other reviewboard) > > What I can do is either one of > 1. Keep the original patch > 2. or create another method, something like > populateActionMetadataServiceCheck(), have the logic below and call it at the > end of fillInfo() > which one do you prefer? > ``` > for(ServiceInfo serviceInfo : stackInfo.getServices()) { > if(serviceInfo.getCommandScript() != null) { > actionMetadata.addServiceCheckAction(serviceInfo.getName()); > } > } > // add service check actions from parents of the target stack > LinkedList<StackInfo> parentStackInfoList = (LinkedList<StackInfo>) > stackParentsMap.get(stackInfo.getVersion()); > for(StackInfo parentStackInfo : parentStackInfoList){ > for(ServiceInfo serviceInfo : parentStackInfo.getServices()) { > if(serviceInfo.getCommandScript() != null) { > actionMetadata.addServiceCheckAction(serviceInfo.getName()); > } > } > } > ``` > > > > why do you need to check parent stack == null? > > We have observed that if there is only one stack version under > /var/lib/ambari-server/resources/stacks/HDP/ and it is active, > ``` > LinkedList<StackInfo> parents = (LinkedList<StackInfo>) > stackParentsMap.get(stackInfo.getVersion()); > ``` > then parents == null.
Why do you need to iterate over the parent of every service? - The Servce check action map is purely a map of service name to action name, which should be added for every service in current stack, irrespective of what the parent has or has not, so in verion 1 of HDFS there is a command script and thereby a HDFS_SERVICE_CHECK, in version 2 you do not need it unless version1 is deleted, in that case version2 of HDFS has to define the command script for it to be present. So in populateServiceInfo all you should need to do is, if(serviceInfo.getCommandScript() != null && actionMetadata.getServiceCheckAction(serviceName) != null)) ... the check is actually not necessary but looks clean. Let me know if this makes sense. - Also, the unit test that you added was valid, I don't see it as a part of the patch. - Sid ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26057/#review55306 ----------------------------------------------------------- On Sept. 29, 2014, 8:44 p.m., Alexander Denissov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26057/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2014, 8:44 p.m.) > > > Review request for Ambari, Mahadev Konar and Newton Alex. > > > Bugs: AMBARI-7442 > https://issues.apache.org/jira/browse/AMBARI-7442 > > > Repository: ambari > > > Description > ------- > > ServiceCheck cannot be run if there is only one stack definition in the stack > family > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/util/StackExtensionHelper.java > fe6c6bc > > ambari-server/src/test/java/org/apache/ambari/server/api/util/StackExtensionHelperTest.java > 74761dd > ambari-server/src/test/resources/single_stack/ABC/1.0.0/metainfo.xml > PRE-CREATION > > ambari-server/src/test/resources/single_stack/ABC/1.0.0/services/HDFS/metainfo.xml > PRE-CREATION > > Diff: https://reviews.apache.org/r/26057/diff/ > > > Testing > ------- > > tested the fix against a cluster > > > Thanks, > > Alexander Denissov > >
