> 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. > > Sid Wagle wrote: > 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.
Correction, actionMetadata.getServiceCheckAction(serviceName) == null - 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 > >
