> On April 6, 2016, 10:47 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java,
> >  line 250
> > <https://reviews.apache.org/r/45811/diff/1/?file=1327547#file1327547line250>
> >
> >     Does 2 really matter here? `stack_version` and `stack_name` ? What if 
> > there's another condition? Can't you still do your predicate magic and 
> > preserve the other conditions?
> >     
> >     I know that it makes the below logic way more complicated since now you 
> > have to deal with which predicated is which, but this could also mask 
> > future problems, no?

Right now the only use case is a direct invocation given a stack name and 
version.  (/api/v1/stacks/*HDP*/versions/*2.2*/compatible_repository_versions). 
 Any other use case falls back to the original behavior.  If we need it being 
more complex then we can revisit.


> On April 6, 2016, 10:47 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java,
> >  lines 237-244
> > <https://reviews.apache.org/r/45811/diff/1/?file=1327547#file1327547line237>
> >
> >     Can you give a concrete example in the documentation so it's clear. Are 
> > we talking about about something like:
> >     
> >     ```
> >     stack_version = 2.2 -> stack_version in (2.2, 2.3, 2.4)
> >     ```

Will do.


> On April 6, 2016, 10:47 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java,
> >  line 950
> > <https://reviews.apache.org/r/45811/diff/1/?file=1327545#file1327545line950>
> >
> >     Safer to just return the original? Would prevent null checks on 
> > invocations of this method.

Thanks for reviewing!  The null check has to be somewhere (in QueryImpl now); 
we can't rely on subclasses always returning non-null values.  
ClusterControllerImpl is really just the pass-through to the provider.


- Nate


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


On April 6, 2016, 10:03 a.m., Nate Cole wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45811/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 10:03 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Jayush 
> Luniya.
> 
> 
> Bugs: AMBARI-15717
>     https://issues.apache.org/jira/browse/AMBARI-15717
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> A previous bug was allowing compatible stacks to come across correctly.  
> After that fix, that made the compatible_repository_version API filter 
> responses that it shouldn't have.  This led to an interesting problem whereby 
> we needed a mechanism to alter the incoming predicate to allow those 
> resources to be included.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/query/QueryImpl.java 
> 36ed189 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java
>  9f864f8 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersion.java
>  1c5fe89 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java
>  26b9645 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ReadOnlyResourceProvider.java
>  a72527d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/spi/ClusterController.java
>  aa1f09f 
>   
> ambari-server/src/test/java/org/apache/ambari/server/api/query/QueryImplTest.java
>  f869ba3 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProviderTest.java
>  faef1c9 
> 
> Diff: https://reviews.apache.org/r/45811/diff/
> 
> 
> Testing
> -------
> 
> Manual.  Automated pending.
> 
> 
> Thanks,
> 
> Nate Cole
> 
>

Reply via email to