> On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterSettingResourceDefinition.java > > Lines 37-42 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884264#file1884264line37> > > > > Why override only to return the same as `super`?
Removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java > > Lines 54 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884267#file1884267line54> > > > > This should point to real interfaces that documents the request > > structure. Not required, removed, as we are not creating any related dependency. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java > > Lines 55 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884267#file1884267line55> > > > > Unused. Not required, removed, as we are not creating any related dependency. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java > > Lines 233-235 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884267#file1884267line233> > > > > Seems incomplete. Can it be completed or removed? Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 68 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line68> > > > > left-over doc ("service"?) Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 84 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line84> > > > > could be final Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 85 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line85> > > > > `Arrays.asList` takes varargs, no need for `new String[]`. > > > > Further, `new HashSet<String>(Arrays.asList(...))` could be simplified > > to `Sets.newHashSet(...)` . Fixed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 89 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line89> > > > > Unused. Removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 114 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line114> > > > > Unused. Removed > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 120 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line120> > > > > Unused. Removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 147-148 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line147> > > > > `= null` assignment is unnecessary. Removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 193 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line193> > > > > no need for explicit type in `HashSet<...>` Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 220-221 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line220> > > > > `= null` assignment is unnecessary. Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 253 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line253> > > > > `= null` is unnecessary, move declaration to first assignment a few > > lines below. Moved and null removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 267 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line267> > > > > `svgReq` name is not appropriate (no service groups here), could be > > simply `req`. Made settingReq > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 304 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line304> > > > > The name `svcRequest` is not appropriate, since it's not a > > `ServiceRequest`. Can be simplified to return without the local variable. Done. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 309 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line309> > > > > could be `private` Fixed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 333 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line333> > > > > typo in "cluste" Done. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 334 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line334> > > > > could be `private` Done. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 358 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line358> > > > > "services"? Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 359 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line359> > > > > could be `private` Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 361 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line361> > > > > no need for explicit type in `HashSet<...>` Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 412 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line412> > > > > could be `private` Updated. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 445 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line445> > > > > unnecessary removed as return is void. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 472 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line472> > > > > No need for explicit type parameter in `new HashSet<...>` Fixed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > > Lines 539 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884276#file1884276line539> > > > > No need for explicit type parameter in `new HashSet<...>` Fixed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java > > Lines 101-102 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884286#file1884286line101> > > > > Please avoid adding useless javadoc (ie. without description) -- same > > below Removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSettingImpl.java > > Lines 156 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884289#file1884289line156> > > > > No `super` method to inherit doc from. Removed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > > Lines 1126 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884291#file1884291line1126> > > > > Explicit type parameter is unnecessary for `new HashMap<...>` Fixed. > On Nov. 8, 2017, 2:02 p.m., Attila Doroszlai wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > > Lines 1653 (patched) > > <https://reviews.apache.org/r/63573/diff/2/?file=1884291#file1884291line1653> > > > > Seems to be left-over reference Removed. - Swapan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63573/#review190435 ----------------------------------------------------------- On Nov. 7, 2017, 11:59 p.m., Swapan Shridhar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63573/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2017, 11:59 p.m.) > > > Review request for Ambari, Jayush Luniya and Madhuvanthi Radhakrishnan. > > > Bugs: AMBARI-22366 > https://issues.apache.org/jira/browse/AMBARI-22366 > > > Repository: ambari > > > Description > ------- > > AMBARI-22366. POST, GET and UPDATE API for cluster settings. > (/clusters/{clusterName}/settings). > > **Background:** AMBARI-22196 did the following : > (1). Refactoring work of moving all cluster related configs/settings in > cluster-env to "cluster-settings" files. > (2). Implementing READ only API to access *cluster-settings* file > *setting(s)* : *http://<host>:<port>/api/v1/cluster_settings/ * > > - This was done to separate cluster related configs from stack related > configs. > > **This JIRA implements the following :** > > - POST, GET, UPDATE and DELETE APIs for cluster settings with endpoint : > *http://<host>:<port>/api/v1/clusters/[[clusterName]]/settings*, where > -- the cluster settings would be initially read from the above mentioned > read-only API : *http://<host>:<port>/api/v1/cluster_settings/*. > > - Further, added cluster *settings* as a sub-resource of *cluster* resource. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/ClusterSettingNotFoundException.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterResourceDefinition.java > e5680dd > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterSettingResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > 6096fa5 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java > 92f50f4 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > 358b1bf > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 370f735 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ClusterSettingRequest.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/ClusterSettingResponse.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > 3db55d4 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ResourceProviderFactory.java > 20f4864 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupResponse.java > 147650c > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java > 9050e3d > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java > c219d23 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java > 2e935af > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > bd6d7bb > > ambari-server/src/main/java/org/apache/ambari/server/events/ClusterSettingEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ClusterSettingDAO.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterEntity.java > abbf709 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterSettingEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterSettingEntityPK.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/resources/RootLevelSettingsManager.java > 3b74e69 > ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java > 5ba61b7 > > ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSetting.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSettingFactory.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSettingImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroupImpl.java > 7737c0d > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > e896d0e > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql 19e23c5 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql b1990df > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 015d6ac > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql d3c7ff6 > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql 6bfe205 > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 65315ec > ambari-server/src/main/resources/META-INF/persistence.xml 009ecd5 > > > Diff: https://reviews.apache.org/r/63573/diff/2/ > > > Testing > ------- > > **Implemented API details:** > > **==================================================** > **POST:** > **=====** > > **Single Setting POST**: > > POST http://{{host1}}:8080/api/v1/clusters/c1/settings/ > > {code:title=REQUEST} > [ > { > "ClusterSettingInfo" : { > "cluster_setting_name": "security_enabled", > "cluster_setting_value": "false" > } > } > ] > {code} > > {code:title=RESPONSE} > { > "resources" : [ > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/security_enabled";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 54, > "cluster_setting_name" : "security_enabled", > "cluster_setting_value" : "false" > } > } > ] > } > {code} > > **Multiple Setting POST**: > > POST http://{{host1}}:8080/api/v1/clusters/c1/settings/ > > {code:title=REQUEST} > [ > { > "ClusterSettingInfo" : { > "cluster_setting_name": "smokeuser", > "cluster_setting_value": "smoke_user1" > } > }, > { > "ClusterSettingInfo" : { > "cluster_setting_name": "ignore_groupsusers_create", > "cluster_setting_value": "false" > } > } > ] > {code} > > {code:title=RESPONSE} > { > "resources" : [ > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/smokeuser";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 55, > "cluster_setting_name" : "smokeuser", > "cluster_setting_value" : "smoke_user1" > } > }, > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/ignore_groupsusers_create";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 56, > "cluster_setting_name" : "ignore_groupsusers_create", > "cluster_setting_value" : "false" > } > } > ] > } > {code} > > > **==================================================** > **GET** > **====** > > **Multiple Setting GET**: > > GET http://{{host1}}:8080/api/v1/clusters/c1/settings/ > > {code:title=REQUEST} > [] > {code} > > {code:title=RESPONSE} > { > "href" : "http://172.22.87.100:8080/api/v1/clusters/c1/settings/";, > "items" : [ > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/ignore_groupsusers_create";, > "ClusterSettingInfo" : { > "cluster_name" : "c1", > "cluster_setting_name" : "ignore_groupsusers_create" > } > }, > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/security_enabled";, > "ClusterSettingInfo" : { > "cluster_name" : "c1", > "cluster_setting_name" : "security_enabled" > } > }, > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/smokeuser";, > "ClusterSettingInfo" : { > "cluster_name" : "c1", > "cluster_setting_name" : "smokeuser" > } > } > ] > } > {code} > > > **Single Setting GET**: > > GET http://{{host1}}:8080/api/v1/clusters/c1/settings/smokeuser > > {code:title=REQUEST} > [] > {code} > > {code:title=RESPONSE} > { > "href" : "http://172.22.87.100:8080/api/v1/clusters/c1/settings/smokeuser";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 55, > "cluster_setting_name" : "smokeuser", > "cluster_setting_value" : "smoke_user1" > } > } > {code} > > > **==================================================** > **PUT** > **====** > > **Single Setting PUT**: > > PUT http://{{host1}}:8080/api/v1/clusters/c1/settings/security_enabled > > {code:title=REQUEST} > [ > { > "ClusterSettingInfo" : { > "cluster_setting_value": "true" > } > } > ] > {code} > > {code:title=RESPONSE} > { > "resources" : [ > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/security_enabled/security_enabled";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 54, > "cluster_setting_name" : "security_enabled", > "cluster_setting_value" : "true" > } > } > ] > } > {code} > > > **Multiple Setting PUT**: > > PUT http://{{host1}}:8080/api/v1/clusters/c1/settings/ > > {code:title=REQUEST} > [ > { > "ClusterSettingInfo" : { > "cluster_setting_name" : "ignore_groupsusers_create", > "cluster_setting_value": "true" > } > }, > { > "ClusterSettingInfo" : { > "cluster_setting_name" : "smokeuser", > "cluster_setting_value": "smoke_user2" > } > } > ] > {code} > > {code:title=RESPONSE} > { > "resources" : [ > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/smokeuser";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 55, > "cluster_setting_name" : "smokeuser", > "cluster_setting_value" : "smoke_user2" > } > }, > { > "href" : > "http://172.22.87.100:8080/api/v1/clusters/c1/settings/ignore_groupsusers_create";, > "ClusterSettingInfo" : { > "cluster_id" : 2, > "cluster_name" : "c1", > "cluster_setting_id" : 56, > "cluster_setting_name" : "ignore_groupsusers_create", > "cluster_setting_value" : "true" > } > } > ] > } > {code} > > > ***==================================================** > **DELETE** > **======** > > DELETE http://{{host1}}:8080/api/v1/clusters/c1/settings/security_enabled > > {code:title=REQUEST} > [] > {code} > > {code:title=RESPONSE} > { > "deleteResult" : [ > { > "deleted" : { > "key" : "cluster_name: c1, cluster_setting_name: security_enabled" > } > } > ] > } > {code} > > > Thanks, > > Swapan Shridhar > >
