ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 28124: Repository Version Management - Create subresource for hosts and clusters
Date Tue, 18 Nov 2014 02:03:56 GMT


> On Nov. 17, 2014, 8:08 p.m., John Speidel wrote:
> > -1
> > Please dion't merge this.
> > 
> > Please point to a design document for this api addition or documentation that describes
the details of this api and why it is needed.
> > Most of this information is already available via the api.
> > 
> > Also, please include Tom Beerbower in all API related reviews.
> 
> Nate Cole wrote:
>     John, no this is something different.  This effort is to support rolling upgrades
of a cluster, where we have to maintain version information for a cluster.  In addition, we
are also going to be tracking which versions have been installed on hosts.  This is a major
feature of 2.0.0 and has already been through significant design.  Having said that, I've
made comments to see if this patch can be simplified a bit.

Nate,
My primary concern was that this is a significant api addition that Tom didn't know about
and has no supporting documentation.  Looking at the Jira and the review, there is no indicaiton
as to what use cases this api addition addresses.  We only know that a 'stack_versions' endpoint
is being added as a subresource of clustsers and hosts and we know the fields available in
the endpoint but not how it is to be used and why it is needed. The -1 was not because I think
that there is an issue with this work, but instead because there is no way to know if this
api is appropriate since it is impossible to know what it is supposed to do.  In my opinion,
no sigificant api changes should ever be merged without documentaiton and/or a design document
that at a minimum descibes the driving use case and provides examples of the api. For this
Jira, all we have is examples of the api.  With this information it is only possible to review
the implementation, not the api.  API changes are a significant
  commitment that last well into the future and must be held to a higher standard that an
implementation because we can't just change the api. 

I am happy that this has gone through significant design, all that I ask is that those design
docs be attached to the Jira so that it is possible to provide a meaningful review of the
api changes.


- John


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28124/#review61787
-----------------------------------------------------------


On Nov. 17, 2014, 6:28 p.m., Yurii Shylov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28124/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2014, 6:28 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Dmitro Lisnichenko, and Nate Cole.
> 
> 
> Bugs: AMBARI-8353
>     https://issues.apache.org/jira/browse/AMBARI-8353
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Add stack_versions subresource to /hosts and /clusters 
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterResourceDefinition.java
ef907c0 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterStackVersionResourceDefinition.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/HostResourceDefinition.java
14ed799 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/HostStackVersionResourceDefinition.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java
9ad37ec 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java
f75ae11 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterStackVersionService.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/HostService.java
c51722c 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/HostStackVersionService.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java
2d91462 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/ResourceProviderFactory.java
212f944 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java
c198dd6 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostStackVersionResourceProvider.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java dbac906

>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ClusterVersionDAO.java
e2a2e2d 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostVersionDAO.java 8d147a1

>   ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterVersionEntity.java
aaf8eed 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostVersionEntity.java
5b1b4f8 
>   ambari-server/src/main/java/org/apache/ambari/server/state/ClusterVersionState.java
72cd541 
>   ambari-server/src/main/java/org/apache/ambari/server/state/RepositoryVersionState.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/api/resources/ClusterResourceDefinitionTest.java
7296e8d 
>   ambari-server/src/test/java/org/apache/ambari/server/api/resources/ClusterStackVersionResourceDefinitionTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/api/resources/HostResourceDefinitionTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/api/resources/HostStackVersionResourceDefinitionTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/api/resources/RepositoryVersionResourceDefinitionTest.java
c0e625a 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/ClusterStackVersionServiceTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/HostStackVersionServiceTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/RepositoryVersionResourceProviderTest.java
e8ce2f7 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/dao/ClusterVersionDAOTest.java
42bf009 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/dao/HostVersionDAOTest.java
93f78b2 
> 
> Diff: https://reviews.apache.org/r/28124/diff/
> 
> 
> Testing
> -------
> 
> in progress
> 
> 
> Thanks,
> 
> Yurii Shylov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message