> On Dec. 22, 2016, 5:32 a.m., Sid Wagle wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java, > > line 250 > > <https://reviews.apache.org/r/54958/diff/1/?file=1590764#file1590764line250> > > > > Suggestion friendlier naming for variable: schResponseList, conveys > > better info than serviceComponentList.
suggestion has been incorporated in the 2nd revision of the patch > On Dec. 22, 2016, 5:32 a.m., Sid Wagle wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java, > > line 299 > > <https://reviews.apache.org/r/54958/diff/1/?file=1590764#file1590764line299> > > > > Isn't this the same as props.putAll(properties.values())? This logic was existing before. I accidently happened to change just the variable name which I have again reverted back to what it was before. To answer the question, I believe its not the same thing. Over here we are flatennig Map<String, Map<String, String>> to Map<String,String> props.putAll(properties.values()) will be passing collection of Map<String, String> to putAll method instead of passing a map: Map<String, String>. Note: putAll API of the map does not accept collection of maps. > On Dec. 22, 2016, 5:32 a.m., Sid Wagle wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java, > > line 240 > > <https://reviews.apache.org/r/54958/diff/1/?file=1590764#file1590764line240> > > > > Shouldn't this just log and error and move on why throw an exception? > > Nate Cole wrote: > +1 We are throwing exception over here only when config files is not present and the response has just one component. For example: http://localhost:8080/api/v1/clusters/c1/services/HDFS/components/DATANODE?format=client_config_tar will throw an exception that "No configuration files defined for the component Datanode" or if any other new client is introduced but no config files are mapped with it in the service metainfo.xml. This is the existing behavior of the API and the patch tries to not change this behavior. if an API call is made to download configs for a service/cluster/host (extended feature added as part of this patch) and none of the beneath components has config files mapped to it then we throw system exception later with a generic message "No configuration files defined for any component". The intention of throwing the exception is to return API with failed error code raher than 200 OK with empty tar file considering python script will not burst when provided with empty config files input. Again this is an existing API behavior and I personally feel it's correct. - Jaimin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54958/#review159924 ----------------------------------------------------------- On Dec. 23, 2016, 12:57 a.m., Jaimin Jetly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54958/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2016, 12:57 a.m.) > > > Review request for Ambari, Jonathan Hurley, Nate Cole, Sumit Mohanty, and Sid > Wagle. > > > Bugs: AMBARI-19275 > https://issues.apache.org/jira/browse/AMBARI-19275 > > > Repository: ambari > > > Description > ------- > > Currently there is an API to download compressed config files for a host > component > > Scope of this ticket is to extend this functionality to do following: > # Download all client config files for a host via single API > # Download all client config files of a service via single API. some services > have more than one client > # Download all client config files for cluster > > Apart from extending the functionalty of API, an additional change has been > made to the behavior of the existing API that looked to be like a bug > Without this patch Downloading client config files for a service component > used to fetch first host component and create config files as present on that > host component (host desired configs overlaid on cluster desired configs) > Instead with a change in this patch when download client config is made on a > service component then config files with only cluster base configs will be > created for downoad. > > > Diffs > ----- > > ambari-server/pom.xml ceec39c > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ComponentService.java > ded2596 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/HostComponentService.java > bc9bb30 > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java > d2f7934 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java > 020a454 > > ambari-server/src/test/java/org/apache/ambari/server/api/services/ComponentServiceTest.java > cb3c2a7 > > ambari-server/src/test/java/org/apache/ambari/server/api/services/HostComponentServiceTest.java > f67d961 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProviderTest.java > cb88ae1 > > Diff: https://reviews.apache.org/r/54958/diff/ > > > Testing > ------- > > Tested manually on a cluster > Added validation to existing unit tests > Executed all unit tests on dev machine and checked there is no regression > Jenkins QA job for 1st revision of the patch compltedt successsfully > Jenkins QA job for 2nd revision of the patch in progress... > > > Thanks, > > Jaimin Jetly > >
