You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Dmitro Lisnichenko <dl...@hortonworks.com> on 2017/08/28 16:18:17 UTC
Review Request 61949: Preview: Reject PATCH VDFs with Services that
are not Included in the Cluster
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/
-----------------------------------------------------------
Review request for Ambari, Jonathan Hurley and Nate Cole.
Bugs: AMBARI-21832
https://issues.apache.org/jira/browse/AMBARI-21832
Repository: ambari
Description
-------
Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
- Install ZooKeeper, Storm on HDP 2.6.0.0-1234
- Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
- Install Accumulo
Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java 77e6250084
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java e81d1af72e
Diff: https://reviews.apache.org/r/61949/diff/1/
Testing
-------
will fix unit tests
Thanks,
Dmitro Lisnichenko
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review184311
-----------------------------------------------------------
Ship it!
Ship It!
- Jonathan Hurley
On Aug. 31, 2017, 1:06 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 1:06 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java 7a53e91bb0
>
>
> Diff: https://reviews.apache.org/r/61949/diff/2/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review184309
-----------------------------------------------------------
Ship it!
Ship It!
- Nate Cole
On Aug. 31, 2017, 1:06 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 1:06 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java 7a53e91bb0
>
>
> Diff: https://reviews.apache.org/r/61949/diff/2/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/
-----------------------------------------------------------
(Updated Aug. 31, 2017, 8:06 p.m.)
Review request for Ambari, Jonathan Hurley and Nate Cole.
Changes
-------
Addressed Nate's comments
Bugs: AMBARI-21832
https://issues.apache.org/jira/browse/AMBARI-21832
Repository: ambari
Description
-------
Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
- Install ZooKeeper, Storm on HDP 2.6.0.0-1234
- Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
- Install Accumulo
Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java 7a53e91bb0
Diff: https://reviews.apache.org/r/61949/diff/2/
Changes: https://reviews.apache.org/r/61949/diff/1-2/
Testing
-------
will fix unit tests
Thanks,
Dmitro Lisnichenko
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
> On Aug. 28, 2017, 7:29 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
> > Lines 545-546 (patched)
> > <https://reviews.apache.org/r/61949/diff/1/?file=1806131#file1806131line545>
> >
> > Can you just @Inject Clusters ?
moved code, now irrelevant
- Dmitro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review183944
-----------------------------------------------------------
On Aug. 31, 2017, 8:06 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 8:06 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java 7a53e91bb0
>
>
> Diff: https://reviews.apache.org/r/61949/diff/2/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review183944
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
Lines 520-523 (patched)
<https://reviews.apache.org/r/61949/#comment259990>
Document the expected behavior of an IllegalArgException being thrown.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
Lines 545-546 (patched)
<https://reviews.apache.org/r/61949/#comment259989>
Can you just @Inject Clusters ?
- Jonathan Hurley
On Aug. 28, 2017, 12:18 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2017, 12:18 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java 77e6250084
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java e81d1af72e
>
>
> Diff: https://reviews.apache.org/r/61949/diff/1/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review183964
-----------------------------------------------------------
I would ask you to consider to move this code to ClusterStackVersionResourceProvider. You're basically checking to see if all the VDFs services are installed, and that should be a cluster check (because we KNOW what services belong to them).
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
Lines 534-538 (patched)
<https://reviews.apache.org/r/61949/#comment260004>
Is there something special about this AmbariException that can't be thrown (as in the method signature)?
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
Lines 548-550 (patched)
<https://reviews.apache.org/r/61949/#comment260005>
This code is implying that all services of all clusters are on the same stack as for the VDF. While this may be true today, it won't always be. We should add some kind of check here.
- Nate Cole
On Aug. 28, 2017, 12:18 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2017, 12:18 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java 77e6250084
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java e81d1af72e
>
>
> Diff: https://reviews.apache.org/r/61949/diff/1/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
> On Aug. 28, 2017, 7:19 p.m., Dmitro Lisnichenko wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
> > Line 87 (original), 91 (patched)
> > <https://reviews.apache.org/r/61949/diff/1/?file=1806131#file1806131line91>
> >
> > May I do this change?
Otherwise I can not access getManagementController()
- Dmitro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review183942
-----------------------------------------------------------
On Aug. 28, 2017, 7:18 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2017, 7:18 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java 77e6250084
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java e81d1af72e
>
>
> Diff: https://reviews.apache.org/r/61949/diff/1/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>
Re: Review Request 61949: Preview: Reject PATCH VDFs with Services
that are not Included in the Cluster
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61949/#review183942
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java
Line 87 (original), 91 (patched)
<https://reviews.apache.org/r/61949/#comment259987>
May I do this change?
- Dmitro Lisnichenko
On Aug. 28, 2017, 7:18 p.m., Dmitro Lisnichenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61949/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2017, 7:18 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-21832
> https://issues.apache.org/jira/browse/AMBARI-21832
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Currently there is an odd scenario which can occur when patch repositories are registered which have services not yet installed. Consider the following scenario:
>
> - Install ZooKeeper, Storm on HDP 2.6.0.0-1234
> - Register/patch a {{PATCH}} VDF for Storm and Accumulo for 2.6.0.1-9999
> - Install Accumulo
>
> Which version does Accumulo use - the {{STANDARD}} repository or the {{PATCH}}? If the {{PATCH}} repository is chosen, this will now prevent reversion of the patch since there's no prior version for Accumulo to revert back to.
>
> If Accumulo uses the {{STANDARD}} repo, then there needs to be a lot of design and UX flow work provided to indicate that a {{PATCH}} which was previously applied can be re-applied for the new service. This also causes problems for patch reversion since now there would be two upgrades which need to be reverted to "get rid" of the patch.
>
> For the timeframe for Ambari 2.6, we should reject VDFs that include services which are not installed. This will prevent the problem.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java 77e6250084
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/VersionDefinitionResourceProvider.java e81d1af72e
>
>
> Diff: https://reviews.apache.org/r/61949/diff/1/
>
>
> Testing
> -------
>
> will fix unit tests
>
>
> Thanks,
>
> Dmitro Lisnichenko
>
>