> On Jan. 7, 2015, 8:23 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackArtifactResourceProvider.java, > > line 91 > > <https://reviews.apache.org/r/29664/diff/1/?file=808794#file808794line91> > > > > It seems like this should be `private` especially since is it not an > > immutable set of string and there is a `protected` member method to get > > this data.
I absolutely agree that it should be private and it originally was but some existing unit tests broke because they needed access to these fields. See ClusterControllerImplTest. These tests use a static test resource and since we don't have an instance of the actual resource provider, we can't use it's accessors. I realize that this is really ugly but drew a line on changing this mechanism and just followed along even though it did cause me some pain to make them public. I could have copied the values but thought it more brittle to copy/paste the set. The other resource providers that I looked at did the same thing, so I guess I just gave in. Since I am not the only one that this bothers, and I am glad about this, I will make them private again and copy/paste the sets as appropriate for these tests or get the data another way. Would you be ok with me making this change in my next patch which is for the other artifact resource providers? > On Jan. 7, 2015, 8:23 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackArtifactResourceProvider.java, > > line 96 > > <https://reviews.apache.org/r/29664/diff/1/?file=808794#file808794line96> > > > > It seems like this should be `private` especially since is it not an > > immutable map. > > > > If the set of key property ids are needed, they can be retrieved using > > `org.apache.ambari.server.controller.internal.AbstractResourceProvider#getKeyPropertyIds` see above comment for pkPropertyIds (line 91) > On Jan. 7, 2015, 8:23 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackArtifactResourceProvider.java, > > line 102 > > <https://reviews.apache.org/r/29664/diff/1/?file=808794#file808794line102> > > > > It seems like this should be `private` especially since is it not an > > immutable set of strings. > > > > If the set of property ids are needed, they can be retrieved using > > `org.apache.ambari.server.controller.internal.BaseProvider#getPropertyIds` see above comment for pkPropertyIds (line 91) > On Jan. 7, 2015, 8:23 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackArtifactResourceProvider.java, > > line 196 > > <https://reviews.apache.org/r/29664/diff/1/?file=808794#file808794line196> > > > > Get all stack and stack service **Kerberos** descriptor resources? thanks - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29664/#review67071 ----------------------------------------------------------- On Jan. 7, 2015, 7:02 p.m., John Speidel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29664/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2015, 7:02 p.m.) > > > Review request for Ambari, Nate Cole, Robert Levas, and Tom Beerbower. > > > Bugs: AMBARI-9028 > https://issues.apache.org/jira/browse/AMBARI-9028 > > > Repository: ambari > > > Description > ------- > > Create new API endpoints for exposing stack and stack service kerberos > descriptors. These endpoints will provide the static descriptors which are > defined in the stack definition and will be immutable. > There will be at most 1 descriptor for both the stack and each stack service. > Therefore, it doesn't make sense to add a kerberos_descriptor sub-resource > since there will be at most one instance. > Instead, a new "artifacts" endpoint is being introduced. > The kerberos descriptor will be an artifact instance. > > See the associated Jira for more information. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > e55b2cb > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/StackServiceResourceDefinition.java > cb1dfb1 > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/StackVersionResourceDefinition.java > e8d4bd5 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/StacksService.java > cdadfd9 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java > 11b0411 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackArtifactResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > 89d6837 > > ambari-server/src/test/java/org/apache/ambari/server/api/query/QueryImplTest.java > 57aea49 > > ambari-server/src/test/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImplTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/api/resources/StackServiceResourceDefinitionTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/api/resources/StackVersionResourceDefinitionTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/api/services/StacksServiceTest.java > 212eaa0 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProviderTest.java > 5414b52 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterControllerImplTest.java > 3ef0ade > > Diff: https://reviews.apache.org/r/29664/diff/ > > > Testing > ------- > > Manual Functional Testing: > queried new resources including collection and instance resources, queries, > partial response and bad requests > > Unit Testing: > All Java unit tests pass > Results : > > Tests run: 2492, Failures: 0, Errors: 0, Skipped: 13 > > Many python test failures which seem unrelated to my patch. > I have sent an email to dev@ambari to determine if these are known issues or > if I need to investigate. > > Also, no unit test was written for the new StackArtifactResourceProvider due > to time constraints. > I have filed the following Jira to track this and ensure that the tests are > written: > https://issues.apache.org/jira/browse/AMBARI-9026 > > > Thanks, > > John Speidel > >
