You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Nate Cole <nc...@hortonworks.com> on 2016/11/28 21:42:15 UTC
Re: Review Request 54086: AMBARI-18987 A general preupgrade check on
if services cannot be upgrade are installed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54086/#review157142
-----------------------------------------------------------
Please add an XML test that shows how to use this.
ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java (line 50)
<https://reviews.apache.org/r/54086/#comment227572>
Spelling: service_removed
ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java (lines 131 - 137)
<https://reviews.apache.org/r/54086/#comment227573>
Should not use CSV here.
ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java (lines 155 - 167)
<https://reviews.apache.org/r/54086/#comment227571>
These exceptions all say the same thing without any hint which condition is triggering it. Should be clarified.
In addition, if these are in xml, then they should be encapsulated in proper xml elements, not CSV.
- Nate Cole
On Nov. 25, 2016, 1:49 p.m., Di Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54086/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2016, 1:49 p.m.)
>
>
> Review request for Ambari and Tim Thorpe.
>
>
> Bugs: AMBARI-18987
> https://issues.apache.org/jira/browse/AMBARI-18987
>
>
> Repository: ambari
>
>
> Description
> -------
>
> A general check that takes list of services defined in upgrade XML and check if they are installed, fail the check if there is at least one hit.
>
> Displaying different error messages depending on if a service does not support upgrade or if a service is removed in the new release ( the two types are specified via different properties in the upgrade XML files.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/CheckDescription.java fbc4be1
> ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/checks/ServicePresenceCheckTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54086/diff/
>
>
> Testing
> -------
>
> new unit tests
> installed an AMbari 2.4.2 cluster, upgrade it to Ambari trunk cluster, update upgrade xml files with the new service check, then run upgrade, verify it fails prechecks accordingly.
>
>
> Thanks,
>
> Di Li
>
>
Re: Review Request 54086: AMBARI-18987 A general preupgrade check on
if services cannot be upgrade are installed
Posted by Nate Cole <nc...@hortonworks.com>.
> On Nov. 28, 2016, 4:42 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java, lines 131-137
> > <https://reviews.apache.org/r/54086/diff/1/?file=1570637#file1570637line131>
> >
> > Should not use CSV here.
Actually, I see now the choice of CSV. You can disregard that, but the Exception text and spelling needs to be fixed
> On Nov. 28, 2016, 4:42 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java, lines 155-167
> > <https://reviews.apache.org/r/54086/diff/1/?file=1570637#file1570637line155>
> >
> > These exceptions all say the same thing without any hint which condition is triggering it. Should be clarified.
> >
> > In addition, if these are in xml, then they should be encapsulated in proper xml elements, not CSV.
Actually, I see now the choice of CSV. You can disregard that, but the Exception text and spelling needs to be fixed
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54086/#review157142
-----------------------------------------------------------
On Nov. 25, 2016, 1:49 p.m., Di Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54086/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2016, 1:49 p.m.)
>
>
> Review request for Ambari and Tim Thorpe.
>
>
> Bugs: AMBARI-18987
> https://issues.apache.org/jira/browse/AMBARI-18987
>
>
> Repository: ambari
>
>
> Description
> -------
>
> A general check that takes list of services defined in upgrade XML and check if they are installed, fail the check if there is at least one hit.
>
> Displaying different error messages depending on if a service does not support upgrade or if a service is removed in the new release ( the two types are specified via different properties in the upgrade XML files.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/CheckDescription.java fbc4be1
> ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/checks/ServicePresenceCheckTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54086/diff/
>
>
> Testing
> -------
>
> new unit tests
> installed an AMbari 2.4.2 cluster, upgrade it to Ambari trunk cluster, update upgrade xml files with the new service check, then run upgrade, verify it fails prechecks accordingly.
>
>
> Thanks,
>
> Di Li
>
>
Re: Review Request 54086: AMBARI-18987 A general preupgrade check on
if services cannot be upgrade are installed
Posted by Di Li <di...@ca.ibm.com>.
> On Nov. 28, 2016, 9:42 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java, lines 155-167
> > <https://reviews.apache.org/r/54086/diff/1/?file=1570637#file1570637line155>
> >
> > These exceptions all say the same thing without any hint which condition is triggering it. Should be clarified.
> >
> > In addition, if these are in xml, then they should be encapsulated in proper xml elements, not CSV.
>
> Nate Cole wrote:
> Actually, I see now the choice of CSV. You can disregard that, but the Exception text and spelling needs to be fixed
Got it. Thanks for the review, Nate. I will update the code accordingly.
- Di
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54086/#review157142
-----------------------------------------------------------
On Nov. 25, 2016, 6:49 p.m., Di Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54086/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2016, 6:49 p.m.)
>
>
> Review request for Ambari and Tim Thorpe.
>
>
> Bugs: AMBARI-18987
> https://issues.apache.org/jira/browse/AMBARI-18987
>
>
> Repository: ambari
>
>
> Description
> -------
>
> A general check that takes list of services defined in upgrade XML and check if they are installed, fail the check if there is at least one hit.
>
> Displaying different error messages depending on if a service does not support upgrade or if a service is removed in the new release ( the two types are specified via different properties in the upgrade XML files.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/CheckDescription.java fbc4be1
> ambari-server/src/main/java/org/apache/ambari/server/checks/ServicePresenceCheck.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/checks/ServicePresenceCheckTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54086/diff/
>
>
> Testing
> -------
>
> new unit tests
> installed an AMbari 2.4.2 cluster, upgrade it to Ambari trunk cluster, update upgrade xml files with the new service check, then run upgrade, verify it fails prechecks accordingly.
>
>
> Thanks,
>
> Di Li
>
>