You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Nate Cole <nc...@hortonworks.com> on 2016/04/06 16:03:44 UTC
Review Request 45811: Compatible Stacks not returning correctly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45811/
-----------------------------------------------------------
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
Re: Review Request 45811: Compatible Stacks not returning correctly
Posted by Jayush Luniya <jl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45811/#review127409
-----------------------------------------------------------
Ship it!
Ship It!
- Jayush Luniya
On April 6, 2016, 2:03 p.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45811/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 2:03 p.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
>
>
Re: Review Request 45811: Compatible Stacks not returning correctly
Posted by Nate Cole <nc...@hortonworks.com>.
> 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
>
>
Re: Review Request 45811: Compatible Stacks not returning correctly
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45811/#review127328
-----------------------------------------------------------
Ship it!
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java (line 950)
<https://reviews.apache.org/r/45811/#comment190601>
Safer to just return the original? Would prevent null checks on invocations of this method.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java (lines 218 - 225)
<https://reviews.apache.org/r/45811/#comment190604>
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)
```
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java (line 231)
<https://reviews.apache.org/r/45811/#comment190611>
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?
- Jonathan Hurley
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
>
>
Re: Review Request 45811: Compatible Stacks not returning correctly
Posted by Nate Cole <nc...@hortonworks.com>.
> On April 6, 2016, 12:06 p.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java, line 75
> > <https://reviews.apache.org/r/45811/diff/1/?file=1327547#file1327547line75>
> >
> > Does this also need "CompatibleRepositoryVersions/"?
> > If not, ship it.
No, it's a marker field only and not meant to be included in the response.
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45811/#review127353
-----------------------------------------------------------
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
>
>
Re: Review Request 45811: Compatible Stacks not returning correctly
Posted by Alejandro Fernandez <af...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45811/#review127353
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CompatibleRepositoryVersionResourceProvider.java (line 70)
<https://reviews.apache.org/r/45811/#comment190632>
Does this also need "CompatibleRepositoryVersions/"?
If not, ship it.
- Alejandro Fernandez
On April 6, 2016, 2:03 p.m., Nate Cole wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45811/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 2:03 p.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
>
>