----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63573/#review190435 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterSettingResourceDefinition.java Lines 37-42 (patched) <https://reviews.apache.org/r/63573/#comment267766> Why override only to return the same as `super`? ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java Lines 54 (patched) <https://reviews.apache.org/r/63573/#comment267768> This should point to real interfaces that documents the request structure. ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java Lines 55 (patched) <https://reviews.apache.org/r/63573/#comment267769> Unused. ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterSettingService.java Lines 233-235 (patched) <https://reviews.apache.org/r/63573/#comment267770> Seems incomplete. Can it be completed or removed? ambari-server/src/main/java/org/apache/ambari/server/controller/ClusterSettingResponse.java Lines 126-141 (patched) <https://reviews.apache.org/r/63573/#comment267771> Can be simplified using `Objects.equals(...)`? ``` return Objects.equals(clusterId, that.clusterId) && Objects.equals(clusterSettingId, that.clusterSettingId) && ... ``` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 68 (patched) <https://reviews.apache.org/r/63573/#comment267772> left-over doc ("service"?) ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 84 (patched) <https://reviews.apache.org/r/63573/#comment267773> could be final ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 85 (patched) <https://reviews.apache.org/r/63573/#comment267774> `Arrays.asList` takes varargs, no need for `new String[]`. Further, `new HashSet<String>(Arrays.asList(...))` could be simplified to `Sets.newHashSet(...)` . ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 89 (patched) <https://reviews.apache.org/r/63573/#comment267775> Unused. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 114 (patched) <https://reviews.apache.org/r/63573/#comment267776> Unused. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 120 (patched) <https://reviews.apache.org/r/63573/#comment267777> Unused. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 147-148 (patched) <https://reviews.apache.org/r/63573/#comment267778> `= null` assignment is unnecessary. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 193 (patched) <https://reviews.apache.org/r/63573/#comment267779> no need for explicit type in `HashSet<...>` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 220-221 (patched) <https://reviews.apache.org/r/63573/#comment267780> `= null` assignment is unnecessary. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 253 (patched) <https://reviews.apache.org/r/63573/#comment267781> `= null` is unnecessary, move declaration to first assignment a few lines below. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 267 (patched) <https://reviews.apache.org/r/63573/#comment267782> `svgReq` name is not appropriate (no service groups here), could be simply `req`. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 274-282 (patched) <https://reviews.apache.org/r/63573/#comment267783> This method always returns an empty `Set`, so it never rejects any property. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 304 (patched) <https://reviews.apache.org/r/63573/#comment267784> The name `svcRequest` is not appropriate, since it's not a `ServiceRequest`. Can be simplified to return without the local variable. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 309 (patched) <https://reviews.apache.org/r/63573/#comment267790> could be `private` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 333 (patched) <https://reviews.apache.org/r/63573/#comment267786> typo in "cluste" ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 334 (patched) <https://reviews.apache.org/r/63573/#comment267788> could be `private` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 358 (patched) <https://reviews.apache.org/r/63573/#comment267785> "services"? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 359 (patched) <https://reviews.apache.org/r/63573/#comment267789> could be `private` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 361 (patched) <https://reviews.apache.org/r/63573/#comment267787> no need for explicit type in `HashSet<...>` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 412 (patched) <https://reviews.apache.org/r/63573/#comment267791> could be `private` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 445 (patched) <https://reviews.apache.org/r/63573/#comment267792> unnecessary ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 461-464 (patched) <https://reviews.apache.org/r/63573/#comment267795> Use `{}` placeholders to avoid unnecessary string concatenation and the need for `isDebugEnabled` check. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 471-473 (patched) <https://reviews.apache.org/r/63573/#comment267796> Can be simplified using `computeIfAbsent` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 472 (patched) <https://reviews.apache.org/r/63573/#comment267797> No need for explicit type parameter in `new HashSet<...>` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 528-531 (patched) <https://reviews.apache.org/r/63573/#comment267794> Use `{}` placeholders to avoid unnecessary string concatenation and the need for `isDebugEnabled` check. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 538-540 (patched) <https://reviews.apache.org/r/63573/#comment267799> Can be simplified using `computeIfAbsent` ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterSettingResourceProvider.java Lines 539 (patched) <https://reviews.apache.org/r/63573/#comment267798> No need for explicit type parameter in `new HashSet<...>` ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterSettingEntity.java Lines 111-115 (patched) <https://reviews.apache.org/r/63573/#comment267801> Can be simplified using `Objects.equals`? ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterSettingEntity.java Lines 120-122 (patched) <https://reviews.apache.org/r/63573/#comment267802> Can be simplified using `Objects.hash(...)` ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterSettingEntityPK.java Lines 27 (patched) <https://reviews.apache.org/r/63573/#comment267825> Why is this PK class needed when it contains a single numeric ID, no composite key? ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ClusterSettingEntityPK.java Lines 55 (patched) <https://reviews.apache.org/r/63573/#comment267803> Use `Objects.hashCode(...)` ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java Lines 101-102 (patched) <https://reviews.apache.org/r/63573/#comment267805> Please avoid adding useless javadoc (ie. without description) -- same below ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSettingImpl.java Lines 150-152 (patched) <https://reviews.apache.org/r/63573/#comment267824> String concatenation in argument to `append()` -- should be several calls instead. ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSettingImpl.java Lines 156 (patched) <https://reviews.apache.org/r/63573/#comment267823> No `super` method to inherit doc from. ambari-server/src/main/java/org/apache/ambari/server/state/ClusterSettingImpl.java Lines 158 (patched) <https://reviews.apache.org/r/63573/#comment267822> "uses Java locks" -- where? ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java Lines 985-989 (patched) <https://reviews.apache.org/r/63573/#comment267820> Use `{}` placeholders instead of string concatenation, then `isDebugEnabled` check is not needed ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java Lines 1009-1012 (patched) <https://reviews.apache.org/r/63573/#comment267821> Use `{}` placeholders instead of string concatenation, then `isDebugEnabled` check is not needed ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java Lines 1126 (patched) <https://reviews.apache.org/r/63573/#comment267819> Explicit type parameter is unnecessary for `new HashMap<...>` ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java Lines 1653 (patched) <https://reviews.apache.org/r/63573/#comment267818> Seems to be left-over reference - Attila Doroszlai On Nov. 8, 2017, 12:59 a.m., Swapan Shridhar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63573/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2017, 12:59 a.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 > >
