You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Nate Cole <nc...@hortonworks.com> on 2014/02/20 17:23:49 UTC
Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/
-----------------------------------------------------------
Review request for Ambari and Tom Beerbower.
Bugs: AMBARI-4761
https://issues.apache.org/jira/browse/AMBARI-4761
Repository: ambari
Description
-------
Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
Diff: https://reviews.apache.org/r/18312/diff/
Testing
-------
* Marked @Ignore on failing test in AmbariManagementControllerTest
* Added test scenarios for multiple hosts
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 12:57.340s
[INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
[INFO] Final Memory: 18M/123M
[INFO] ------------------------------------------------------------------------
Thanks,
Nate Cole
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Nate Cole <nc...@hortonworks.com>.
> On Feb. 20, 2014, 1:20 p.m., Tom Beerbower wrote:
> > In HostResourceProvider it looks like you track a set of cluster names for hosts that are in implied passive state. So, even though we only support a single cluster now, this code can deal with multiple clusters. It looks like in ServiceResourceProvider you have similar code but always assign the cluster name to a single property, potentially overwriting the previous value. Again, since we only allow a single cluster now it shouldn't matter. Just curious why you handled it differently in the two providers? I think it better to account for the multiple cluster names now so that we don't have to deal with it down the line.
Great question. The code in ServiceResourceProvider and AMC (for HostComponents) each have a block (around line 1482 in the diff) that is already throwing an Exception when trying to update more than one cluster. I agree with you - I'll modify the code to just use clusternames set already defined.
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/#review35022
-----------------------------------------------------------
On Feb. 20, 2014, 11:23 a.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18312/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:23 a.m.)
>
>
> Review request for Ambari and Tom Beerbower.
>
>
> Bugs: AMBARI-4761
> https://issues.apache.org/jira/browse/AMBARI-4761
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
> ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
> ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
> ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
>
> Diff: https://reviews.apache.org/r/18312/diff/
>
>
> Testing
> -------
>
> * Marked @Ignore on failing test in AmbariManagementControllerTest
> * Added test scenarios for multiple hosts
>
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 12:57.340s
> [INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
> [INFO] Final Memory: 18M/123M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Nate Cole
>
>
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Nate Cole <nc...@hortonworks.com>.
> On Feb. 20, 2014, 1:20 p.m., Tom Beerbower wrote:
> > In HostResourceProvider it looks like you track a set of cluster names for hosts that are in implied passive state. So, even though we only support a single cluster now, this code can deal with multiple clusters. It looks like in ServiceResourceProvider you have similar code but always assign the cluster name to a single property, potentially overwriting the previous value. Again, since we only allow a single cluster now it shouldn't matter. Just curious why you handled it differently in the two providers? I think it better to account for the multiple cluster names now so that we don't have to deal with it down the line.
>
> Nate Cole wrote:
> Great question. The code in ServiceResourceProvider and AMC (for HostComponents) each have a block (around line 1482 in the diff) that is already throwing an Exception when trying to update more than one cluster. I agree with you - I'll modify the code to just use clusternames set already defined.
Correction - to use a set of clusternames, not the one already defined. Not all clusters may require the update.
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/#review35022
-----------------------------------------------------------
On Feb. 20, 2014, 11:23 a.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18312/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:23 a.m.)
>
>
> Review request for Ambari and Tom Beerbower.
>
>
> Bugs: AMBARI-4761
> https://issues.apache.org/jira/browse/AMBARI-4761
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
> ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
> ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
> ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
>
> Diff: https://reviews.apache.org/r/18312/diff/
>
>
> Testing
> -------
>
> * Marked @Ignore on failing test in AmbariManagementControllerTest
> * Added test scenarios for multiple hosts
>
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 12:57.340s
> [INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
> [INFO] Final Memory: 18M/123M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Nate Cole
>
>
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/#review35022
-----------------------------------------------------------
In HostResourceProvider it looks like you track a set of cluster names for hosts that are in implied passive state. So, even though we only support a single cluster now, this code can deal with multiple clusters. It looks like in ServiceResourceProvider you have similar code but always assign the cluster name to a single property, potentially overwriting the previous value. Again, since we only allow a single cluster now it shouldn't matter. Just curious why you handled it differently in the two providers? I think it better to account for the multiple cluster names now so that we don't have to deal with it down the line.
- Tom Beerbower
On Feb. 20, 2014, 4:23 p.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18312/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 4:23 p.m.)
>
>
> Review request for Ambari and Tom Beerbower.
>
>
> Bugs: AMBARI-4761
> https://issues.apache.org/jira/browse/AMBARI-4761
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
> ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
> ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
> ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
>
> Diff: https://reviews.apache.org/r/18312/diff/
>
>
> Testing
> -------
>
> * Marked @Ignore on failing test in AmbariManagementControllerTest
> * Added test scenarios for multiple hosts
>
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 12:57.340s
> [INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
> [INFO] Final Memory: 18M/123M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Nate Cole
>
>
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Nate Cole <nc...@hortonworks.com>.
> On Feb. 20, 2014, 1:05 p.m., Tom Beerbower wrote:
> > Was the test failing before your changes or because of them? If it's failing because of them, do you think that the test is bogus? If so, I would completely remove the test instead of ignoring it. Also, if you are going to @Ignore a test I would add the annotation to the @Test annotation instead of replacing it.
It was failing before my changes. I @Ignored it because I just needed to make sure agent tests pass.
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/#review35016
-----------------------------------------------------------
On Feb. 20, 2014, 11:23 a.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18312/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:23 a.m.)
>
>
> Review request for Ambari and Tom Beerbower.
>
>
> Bugs: AMBARI-4761
> https://issues.apache.org/jira/browse/AMBARI-4761
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
> ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
> ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
> ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
>
> Diff: https://reviews.apache.org/r/18312/diff/
>
>
> Testing
> -------
>
> * Marked @Ignore on failing test in AmbariManagementControllerTest
> * Added test scenarios for multiple hosts
>
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 12:57.340s
> [INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
> [INFO] Final Memory: 18M/123M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Nate Cole
>
>
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/#review35016
-----------------------------------------------------------
Was the test failing before your changes or because of them? If it's failing because of them, do you think that the test is bogus? If so, I would completely remove the test instead of ignoring it. Also, if you are going to @Ignore a test I would add the annotation to the @Test annotation instead of replacing it.
- Tom Beerbower
On Feb. 20, 2014, 4:23 p.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18312/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 4:23 p.m.)
>
>
> Review request for Ambari and Tom Beerbower.
>
>
> Bugs: AMBARI-4761
> https://issues.apache.org/jira/browse/AMBARI-4761
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
> ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
> ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
> ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
>
> Diff: https://reviews.apache.org/r/18312/diff/
>
>
> Testing
> -------
>
> * Marked @Ignore on failing test in AmbariManagementControllerTest
> * Added test scenarios for multiple hosts
>
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 12:57.340s
> [INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
> [INFO] Final Memory: 18M/123M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Nate Cole
>
>
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/#review35047
-----------------------------------------------------------
Ship it!
Ship It!
- Tom Beerbower
On Feb. 20, 2014, 7:07 p.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18312/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 7:07 p.m.)
>
>
> Review request for Ambari and Tom Beerbower.
>
>
> Bugs: AMBARI-4761
> https://issues.apache.org/jira/browse/AMBARI-4761
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
> ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
> ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
> ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
>
> Diff: https://reviews.apache.org/r/18312/diff/
>
>
> Testing
> -------
>
> * Marked @Ignore on failing test in AmbariManagementControllerTest
> * Added test scenarios for multiple hosts
>
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 12:57.340s
> [INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
> [INFO] Final Memory: 18M/123M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Nate Cole
>
>
Re: Review Request 18312: A single API call to toggle Maintenance Mode for
multiple hosts is creating multiple requests
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18312/
-----------------------------------------------------------
(Updated Feb. 20, 2014, 2:07 p.m.)
Review request for Ambari and Tom Beerbower.
Changes
-------
Updated diff
Bugs: AMBARI-4761
https://issues.apache.org/jira/browse/AMBARI-4761
Repository: ambari
Description
-------
Small change to collect cluster names for maintenance state, then issue only one command, instead of one-per-change.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java d607eb4
ambari-server/src/main/java/org/apache/ambari/server/controller/PassiveStateHelper.java 4d412f7
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java 01163b7
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 9cbd16f
ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java 2a1cf20
ambari-server/src/test/java/org/apache/ambari/server/controller/PassiveStateHelperTest.java 8b4ad30
Diff: https://reviews.apache.org/r/18312/diff/
Testing
-------
* Marked @Ignore on failing test in AmbariManagementControllerTest
* Added test scenarios for multiple hosts
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 12:57.340s
[INFO] Finished at: Thu Feb 20 10:39:21 EST 2014
[INFO] Final Memory: 18M/123M
[INFO] ------------------------------------------------------------------------
Thanks,
Nate Cole