----------------------------------------------------------- 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 > >
