> On June 6, 2014, 2:29 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 2281 > > <https://reviews.apache.org/r/22271/diff/1/?file=604196#file604196line2281> > > > > This method only acts on YARN/NODEMANAGER, yet it doesn't indicate that > > in the method name or the comments. Also, you're hard coding > > service/component names and a DECOMISSION request; seems like this method > > is very specific to a remove host component request.
Like the previous one, this too is caused by my insufficient git skills. The function had been removed with check-in 8ec1837c3de8defafdc3074e68d38cb2bce65f61 for bug ambari-5544. This is not part of my original change, so feel free to ignore it. I'll make sure it doesn't go into the final patch. > On June 6, 2014, 2:29 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 1824 > > <https://reviews.apache.org/r/22271/diff/1/?file=604196#file604196line1824> > > > > Any reason you removed logging? Apparently this is caused by my insufficient git skills. The line has been added with check-in 949ecd21b4001ccd763ad82985ec6efd96eef6ff for bug AMBARI-5761. This is not part of my original change, so feel free to ignore it. I'll make sure it doesn't go into the final patch. > On June 6, 2014, 2:29 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 586 > > <https://reviews.apache.org/r/22271/diff/1/?file=604196#file604196line586> > > > > I don't think that this if-statement matches your comment. If you add > > host components to an existing host in the cluster, then for ZooKeeper, the > > host component map will contain hostName. > > > > This statement will pass when deleting a host and when adding new > > components to an existing host. Is this the side-effect you wanted? I still > > seems that ZooKeeper is indicating a restart in this case. Yes, this is the side effect. Looking at how the function is being used, the host component map update occurs after setRestartRequiredServices is called, in both createHostComponents and deleteHostComponents. - Florian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22271/#review44895 ----------------------------------------------------------- On June 6, 2014, 6:40 p.m., Florian Barca wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22271/ > ----------------------------------------------------------- > > (Updated June 6, 2014, 6:40 p.m.) > > > Review request for Ambari, Dmytro Sen, Jonathan Hurley, and Mahadev Konar. > > > Bugs: AMBARI-6039 > https://issues.apache.org/jira/browse/AMBARI-6039 > > > Repository: ambari > > > Description > ------- > > Added a filtering layer by host name. When adding a host, this is the new > host name, which is not yet available in the configuration, so the filtering > avoids setting the stale flag on the applicable services. > > Remarks: > - The code gets called whenever we add a host, including at cluster setup > time. Didn't notice a change in behavior though. > - The code is trying to cover all the possible situations with one general > approach. There is a potential for future bugs due to new services' > potentially different behavior. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 1297f6c > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 5cf5986 > > Diff: https://reviews.apache.org/r/22271/diff/ > > > Testing > ------- > > Unit test code added in the existing test case. Also, ran the end-to-end test > on the local cluster and confirmed that the ZK service does not appear stale > after adding a host to the existing configuration. > > > Thanks, > > Florian Barca > >
