> On Dec. 23, 2016, 3:48 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java, > > lines 505-516 > > <https://reviews.apache.org/r/54958/diff/2/?file=1591485#file1591485line505> > > > > Since this is one big if/else, can you calcuate the file name in here > > and then instantiate the TarUtils once outside of it?
Addressed in new revision of the patch > On Dec. 23, 2016, 3:48 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java, > > line 499 > > <https://reviews.apache.org/r/54958/diff/2/?file=1591485#file1591485line499> > > > > Do you need to prefix the executeCommands with `this` ? Not actually. It's a habit I carried from JavaScript coding practice. I understand it's confusing in Java. I have addresed this in new revision of the patch > On Dec. 23, 2016, 3:48 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClientConfigResourceProvider.java, > > line 503 > > <https://reviews.apache.org/r/54958/diff/2/?file=1591485#file1591485line503> > > > > Same as above ... remove `this` I have addresed this in new revision of the patch - Jaimin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54958/#review160087 ----------------------------------------------------------- On Dec. 27, 2016, 4:16 p.m., Jaimin Jetly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54958/ > ----------------------------------------------------------- > > (Updated Dec. 27, 2016, 4:16 p.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 102cb71 > > 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 > >
