> On Sept. 8, 2015, 5:14 p.m., Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java, > > line 73 > > <https://reviews.apache.org/r/38182/diff/1/?file=1065274#file1065274line73> > > > > I'm a little concerned with this modification, as it will affect > > basically every resource type managed by Ambari. > > > > We probably should not make a change that could potentially affect API > > compatibility in the "v1" version of the API. > > > > While it would be a less-generic solution, I think I would recommend > > building a solution that is specific to the Blueprints issue, to reduce the > > risk of creating an API incompatibility for all resource types managed by > > ambari-server. > > > > John Speidel should probably comment on this further, since he was the > > lead on the API services in Ambari.
I agree with @rnettleton, however I do like the way this solution attempts to disambiguate a resource's name when this scenario occurs. Wouldn't a more _local_ solution be to simply assume that the name in the URL overrides the name in the JSON document? Or is it too late by the time Ambari knows that the resource being created it a Blueprint? - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38182/#review98064 ----------------------------------------------------------- On Sept. 9, 2015, 11:01 a.m., Sandor Magyari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38182/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2015, 11:01 a.m.) > > > Review request for Ambari, Robert Levas and Robert Nettleton. > > > Bugs: AMBARI-12989 > https://issues.apache.org/jira/browse/AMBARI-12989 > > > Repository: ambari > > > Description > ------- > > PROBLEM > > When creating any resource like blueprint, cluster etc, you can specify the > resource name both in the Url and Json payload as well. For example in case > of a blueprint creation: api/v1/blueprints/sandboxURL Json: > {"Blueprints":{"blueprint_name":"sandboxJson"}}. In this case the blueprint > will be created as sandboxJson so the name specified in Json will override > the name specifie din the Url. The behavoir is true for cluster creation and > any other resource where you can specify name in Json as well. This could be > a bit misleading so the API may return an error in case of resource names > don't match. > > SOLUTION > > The solution would be to throw an error message (Error code 400) in case > resource names are different. To achive this only for cluster creation would > be quite complicated since resource handling is quite generic and in > ClusterResourceProvider - which contains resource specific handling - only > the request body is available, the url is not. The actual behaviour is that > in PersistenceManagerImpl.create method the resource name (eg. > blueprint_name) is inserted from the url in Json in case it's missing. So if > resource name is specified in url than that will be used, if is specified > both url and Json then latter will be used. Because of this behaviour I think > we should hadnle this problem in PersistenceManagerImpl.create as you can see > in attached patch. This will affect of course the creation of all resources. > All unittests were > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java > 4db5611 > > ambari-server/src/test/java/org/apache/ambari/server/api/services/PersistenceManagerImplTest.java > 9ff1506 > > Diff: https://reviews.apache.org/r/38182/diff/ > > > Testing > ------- > > Manually tested blueprint and cluster creation, then cluster creation with > blueprint. All unit tests passed, it seems it doesn't affect normal > functionality. > > > Thanks, > > Sandor Magyari > >
