Mailing-List: contact dev-help@ambari.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@ambari.apache.org
Content-Type: multipart/alternative;
 boundary="===============3492588631590805145=="
MIME-Version: 1.0
Subject: Re: Review Request 22271: ZK should not be required to be restarted
 after
 adding a host
From: "Jonathan Hurley" <jhurley@hortonworks.com>
To: "Mahadev Konar" <mahadev@apache.org>, "Dmytro Sen" <dsen@hortonworks.com>,
 "Jonathan Hurley" <jhurley@hortonworks.com>
Cc: "Ambari" <dev@ambari.apache.org>, "Florian Barca" <fbarca@hortonworks.com>
Date: Fri, 06 Jun 2014 14:29:55 -0000
Message-ID: <20140606142955.10017.17184@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jonathan Hurley" <noreply@reviews.apache.org>
References: <20140605174411.12450.96564@reviews.apache.org>
In-Reply-To: <20140605174411.12450.96564@reviews.apache.org>
Reply-To: "Jonathan Hurley" <jhurley@hortonworks.com>

--===============3492588631590805145==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22271/#review44895
-----------------------------------------------------------


ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
<https://reviews.apache.org/r/22271/#comment79481>

    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.


ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
<https://reviews.apache.org/r/22271/#comment79482>

    Any reason you removed logging?


ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
<https://reviews.apache.org/r/22271/#comment79483>

    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.


- Jonathan Hurley


On June 5, 2014, 1:44 p.m., Florian Barca wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22271/
> -----------------------------------------------------------
> 
> (Updated June 5, 2014, 1:44 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 8fbf207 
> 
> 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
> 
>


--===============3492588631590805145==--