You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Florian Barca <fb...@hortonworks.com> on 2014/06/05 19:44:11 UTC
Review Request 22271: ZK should not be required to be restarted after adding
a host
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22271/
-----------------------------------------------------------
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
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22271/#review45001
-----------------------------------------------------------
Ship it!
Ship It!
- Jonathan Hurley
On June 6, 2014, 2: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, 2: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
>
>
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Florian Barca <fb...@hortonworks.com>.
> On June 9, 2014, 8:56 p.m., Sumit Mohanty wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, line 586
> > <https://reviews.apache.org/r/22271/diff/2/?file=604826#file604826line586>
> >
> > This might break Nagios behavior as Nagios may not be installed on the host that is being deleted. But Nagios should get restarted when any host is deleted.
It's possible indeed, though my preference would be to create a separate JIRA item to track this one.
> On June 9, 2014, 8:56 p.m., Sumit Mohanty wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, line 579
> > <https://reviews.apache.org/r/22271/diff/2/?file=604826#file604826line579>
> >
> > Minor: The formatting should match the rest of the code - "space" required after if. If you are using IntelliJ then you can set the option in the editor properties.
I think the patch is submitted by now, so I'll remember to fix it next time.
- Florian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22271/#review45133
-----------------------------------------------------------
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
>
>
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Sumit Mohanty <sm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22271/#review45133
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
<https://reviews.apache.org/r/22271/#comment79798>
Minor: The formatting should match the rest of the code - "space" required after if. If you are using IntelliJ then you can set the option in the editor properties.
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
<https://reviews.apache.org/r/22271/#comment79800>
This might break Nagios behavior as Nagios may not be installed on the host that is being deleted. But Nagios should get restarted when any host is deleted.
- Sumit Mohanty
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
>
>
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Florian Barca <fb...@hortonworks.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated patch to reflect the latest branch changes. Removed the parasitic changes that sneaked in the previous patch.
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 (updated)
-----
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
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Florian Barca <fb...@hortonworks.com>.
> 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
>
>
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 22271: ZK should not be required to be restarted after
adding a host
Posted by Dmytro Sen <ds...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22271/#review44893
-----------------------------------------------------------
Ship it!
The patch looks good. All tests passed
- Dmytro Sen
On June 5, 2014, 5: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, 5: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
>
>