You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Tom Beerbower <tb...@hortonworks.com> on 2015/04/10 16:08:18 UTC
Review Request 33079: Views : Auto create
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33079/
-----------------------------------------------------------
Review request for Ambari, Jonathan Hurley, John Speidel, and Robert Levas.
Bugs: AMBARI-10424
https://issues.apache.org/jira/browse/AMBARI-10424
Repository: ambari
Description
-------
Ability to automatically instantiate a view instance for a cluster during create based on the stack and services included in the cluster – whether using blueprints or install wizard (so views are created automatically with a cluster create). The view itself should describe what stack and services it is associated with, and when a cluster is created in the ambari the view is deploy in, a view instance is automatically created. Ambari Admins should have an option to override this setting in case they do not want the view instances automatically created. This includes support for designating in the view.xml stack and services support for a view. Similar to the way min-ambari-version is specified, the view developer needs a way to identify that the view works with stack(s) (such as "HDP-2.2" or "HDP-2.2, HDP 2.3" or "HDP-2*") and what services need to be in the cluster ("YARN" or" YARN, HIVE"). Once a cluster is created matching those stack + service requirements, the view is automatic
ally instantiated.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 2724a97
ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java 1ae1dfd
ambari-server/src/main/java/org/apache/ambari/server/view/configuration/AutoInstanceConfig.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java c617b7f
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 9ec1610
ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java 3a57b1b
ambari-server/src/test/java/org/apache/ambari/server/view/configuration/AutoInstanceConfigTest.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java 1875238
ambari-views/src/main/resources/view.xsd b5ed669
Diff: https://reviews.apache.org/r/33079/diff/
Testing
-------
Manual tested that adding service will trigger auto creation and that the new view instance is associated with that cluster.
New unit tests added.
All tests pass.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 34:51 min
[INFO] Finished at: 2015-04-09T17:28:29-04:00
[INFO] Final Memory: 52M/603M
[INFO] ------------------------------------------------------------------------
Thanks,
Tom Beerbower
Re: Review Request 33079: Views : Auto create
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33079/
-----------------------------------------------------------
(Updated April 10, 2015, 6:31 p.m.)
Review request for Ambari, Jonathan Hurley, John Speidel, and Robert Levas.
Changes
-------
update diff
Bugs: AMBARI-10424
https://issues.apache.org/jira/browse/AMBARI-10424
Repository: ambari
Description
-------
Ability to automatically instantiate a view instance for a cluster during create based on the stack and services included in the cluster – whether using blueprints or install wizard (so views are created automatically with a cluster create). The view itself should describe what stack and services it is associated with, and when a cluster is created in the ambari the view is deploy in, a view instance is automatically created. Ambari Admins should have an option to override this setting in case they do not want the view instances automatically created. This includes support for designating in the view.xml stack and services support for a view. Similar to the way min-ambari-version is specified, the view developer needs a way to identify that the view works with stack(s) (such as "HDP-2.2" or "HDP-2.2, HDP 2.3" or "HDP-2*") and what services need to be in the cluster ("YARN" or" YARN, HIVE"). Once a cluster is created matching those stack + service requirements, the view is automatic
ally instantiated.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 2724a97
ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java 1ae1dfd
ambari-server/src/main/java/org/apache/ambari/server/view/configuration/AutoInstanceConfig.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java c617b7f
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 9ec1610
ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java 3a57b1b
ambari-server/src/test/java/org/apache/ambari/server/view/configuration/AutoInstanceConfigTest.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java 1875238
ambari-views/src/main/resources/view.xsd b5ed669
Diff: https://reviews.apache.org/r/33079/diff/
Testing
-------
Manual tested that adding service will trigger auto creation and that the new view instance is associated with that cluster.
New unit tests added.
All tests pass.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 34:51 min
[INFO] Finished at: 2015-04-09T17:28:29-04:00
[INFO] Final Memory: 52M/603M
[INFO] ------------------------------------------------------------------------
Thanks,
Tom Beerbower
Re: Review Request 33079: Views : Auto create
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On April 10, 2015, 2:56 p.m., Robert Levas wrote:
> > No responses are necessary, but I added some notes and some nit-picky comments.
Thanks for the review!
> On April 10, 2015, 2:56 p.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java, line 174
> > <https://reviews.apache.org/r/33079/diff/1/?file=923100#file923100line174>
> >
> > The ViewRegistry appears to be injected. Do we also want to pass in the a ViewRegistry via a constructor? I suppose nothing is really wrong here.
Yeah, I added the constructor just to make it easy to set the viewRegistry for the tests. I didn't really like doing that so I'm removing it and using your other suggestion to set the field directly.
> On April 10, 2015, 2:56 p.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java, line 912
> > <https://reviews.apache.org/r/33079/diff/1/?file=923101#file923101line912>
> >
> > Couldn't this be another AND clause to the above if statement?
> >
> > Also, if serviceName exists in serviceNames, wouldn't this check also include the check for `autoCreateServices.contains(serviceName)`?
Okay, sure. I made the change to a single if statement.
The reason for the extra check is to make sure that the service that triggered the check is actually one that the view cares about. Otherwise everytime any service was added, it would cause us to check serviceNames.containsAll(autoCreateServices) and potentially add multiple auto create view instances.
> On April 10, 2015, 2:56 p.m., Robert Levas wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java, line 2022
> > <https://reviews.apache.org/r/33079/diff/1/?file=923104#file923104line2022>
> >
> > If this is the only usage of setting the ViewRegistry using the constructor, maybe use `getDeclaredField` instead - like it is done for the injected KerberosHelper.
Good tip! I used this for the test instead of the new constructor.
> On April 10, 2015, 2:56 p.m., Robert Levas wrote:
> > ambari-views/src/main/resources/view.xsd, line 231
> > <https://reviews.apache.org/r/33079/diff/1/?file=923108#file923108line231>
> >
> > There is a `list` type in the XSD namespace that declares a _space_ delimited list of values. I don't think it really makes a difference here, but just a FYI.
I didn't know that. I think that I like the comma delimited list better unless you think that it is inconsistent with other configuration that we have or is non-standard in some way.
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33079/#review79680
-----------------------------------------------------------
On April 10, 2015, 2:08 p.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33079/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 2:08 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley, John Speidel, and Robert Levas.
>
>
> Bugs: AMBARI-10424
> https://issues.apache.org/jira/browse/AMBARI-10424
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Ability to automatically instantiate a view instance for a cluster during create based on the stack and services included in the cluster – whether using blueprints or install wizard (so views are created automatically with a cluster create). The view itself should describe what stack and services it is associated with, and when a cluster is created in the ambari the view is deploy in, a view instance is automatically created. Ambari Admins should have an option to override this setting in case they do not want the view instances automatically created. This includes support for designating in the view.xml stack and services support for a view. Similar to the way min-ambari-version is specified, the view developer needs a way to identify that the view works with stack(s) (such as "HDP-2.2" or "HDP-2.2, HDP 2.3" or "HDP-2*") and what services need to be in the cluster ("YARN" or" YARN, HIVE"). Once a cluster is created matching those stack + service requirements, the view is automat
ically instantiated.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 2724a97
> ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java 1ae1dfd
> ambari-server/src/main/java/org/apache/ambari/server/view/configuration/AutoInstanceConfig.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java c617b7f
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 9ec1610
> ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java 3a57b1b
> ambari-server/src/test/java/org/apache/ambari/server/view/configuration/AutoInstanceConfigTest.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java 1875238
> ambari-views/src/main/resources/view.xsd b5ed669
>
> Diff: https://reviews.apache.org/r/33079/diff/
>
>
> Testing
> -------
>
> Manual tested that adding service will trigger auto creation and that the new view instance is associated with that cluster.
> New unit tests added.
> All tests pass.
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 34:51 min
> [INFO] Finished at: 2015-04-09T17:28:29-04:00
> [INFO] Final Memory: 52M/603M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Tom Beerbower
>
>
Re: Review Request 33079: Views : Auto create
Posted by Robert Levas <rl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33079/#review79680
-----------------------------------------------------------
Ship it!
No responses are necessary, but I added some notes and some nit-picky comments.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java
<https://reviews.apache.org/r/33079/#comment129200>
The ViewRegistry appears to be injected. Do we also want to pass in the a ViewRegistry via a constructor? I suppose nothing is really wrong here.
ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java
<https://reviews.apache.org/r/33079/#comment129201>
Couldn't this be another AND clause to the above if statement?
Also, if serviceName exists in serviceNames, wouldn't this check also include the check for `autoCreateServices.contains(serviceName)`?
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java
<https://reviews.apache.org/r/33079/#comment129202>
If this is the only usage of setting the ViewRegistry using the constructor, maybe use `getDeclaredField` instead - like it is done for the injected KerberosHelper.
ambari-views/src/main/resources/view.xsd
<https://reviews.apache.org/r/33079/#comment129203>
There is a `list` type in the XSD namespace that declares a _space_ delimited list of values. I don't think it really makes a difference here, but just a FYI.
- Robert Levas
On April 10, 2015, 10:08 a.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33079/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 10:08 a.m.)
>
>
> Review request for Ambari, Jonathan Hurley, John Speidel, and Robert Levas.
>
>
> Bugs: AMBARI-10424
> https://issues.apache.org/jira/browse/AMBARI-10424
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Ability to automatically instantiate a view instance for a cluster during create based on the stack and services included in the cluster – whether using blueprints or install wizard (so views are created automatically with a cluster create). The view itself should describe what stack and services it is associated with, and when a cluster is created in the ambari the view is deploy in, a view instance is automatically created. Ambari Admins should have an option to override this setting in case they do not want the view instances automatically created. This includes support for designating in the view.xml stack and services support for a view. Similar to the way min-ambari-version is specified, the view developer needs a way to identify that the view works with stack(s) (such as "HDP-2.2" or "HDP-2.2, HDP 2.3" or "HDP-2*") and what services need to be in the cluster ("YARN" or" YARN, HIVE"). Once a cluster is created matching those stack + service requirements, the view is automat
ically instantiated.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 2724a97
> ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java 1ae1dfd
> ambari-server/src/main/java/org/apache/ambari/server/view/configuration/AutoInstanceConfig.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java c617b7f
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 9ec1610
> ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java 3a57b1b
> ambari-server/src/test/java/org/apache/ambari/server/view/configuration/AutoInstanceConfigTest.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java 1875238
> ambari-views/src/main/resources/view.xsd b5ed669
>
> Diff: https://reviews.apache.org/r/33079/diff/
>
>
> Testing
> -------
>
> Manual tested that adding service will trigger auto creation and that the new view instance is associated with that cluster.
> New unit tests added.
> All tests pass.
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 34:51 min
> [INFO] Finished at: 2015-04-09T17:28:29-04:00
> [INFO] Final Memory: 52M/603M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Tom Beerbower
>
>