> On Aug. 9, 2016, 6:22 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java,
> >  line 21
> > <https://reviews.apache.org/r/50886/diff/1/?file=1465975#file1465975line21>
> >
> >     Could you adjust your import sorting in your IDE to match most of the 
> > files? This helps cut down on merge issues with imports. It should be:
> >     
> >     java
> >     javax
> >     org
> >     com

Thanks for pointng out the sequence. That is helpful.
I will do this in next revision of the patch that I submit to this reviewboard


> On Aug. 9, 2016, 6:22 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java,
> >  lines 3558-3565
> > <https://reviews.apache.org/r/50886/diff/1/?file=1465975#file1465975line3558>
> >
> >     This logic will not use an optimized query for `&is_current=true` when 
> > no service name is present. When asking for the latest service configs of 
> > all services, do we need another query to handle it?

I believe so. 
Currently ambari-web does not query to get all service's active service config 
versions. It always uses "in" operator for multiple services which in backend 
gets splitted as multiple requests each with a equals predicate of single 
service name. 

We can also choose to always get active service config versions for all 
services and then filter the correct ones when serviceName is present in equals 
predicate in the request or can implement a seperate optimized query for all 
service's case.
I will create a seperate ticket to track this particular scenario.


> On Aug. 9, 2016, 6:22 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java,
> >  lines 2733-2735
> > <https://reviews.apache.org/r/50886/diff/1/?file=1465981#file1465981line2733>
> >
> >     Why not use the simpler constructor for ConfigurationResponse which 
> > takes a Config instance?

I will do that in next revision of the patch that I submit to this reviewboard


> On Aug. 9, 2016, 6:22 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ServiceConfigEntity.java,
> >  lines 53-54
> > <https://reviews.apache.org/r/50886/diff/1/?file=1465979#file1465979line53>
> >
> >     I think that using serviceConfig.version instead of timestamp is more 
> > reliable here. System time is easily broke with the failure of the ntpd 
> > service.

I will do that in next revision of the patch that I submit to this reviewboard


- Jaimin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50886/#review145229
-----------------------------------------------------------


On Aug. 8, 2016, 7:10 a.m., Jaimin Jetly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50886/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2016, 7:10 a.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Myroslav 
> Papirkovskyy, Sumit Mohanty, and Sid Wagle.
> 
> 
> Bugs: AMBARI-18055
>     https://issues.apache.org/jira/browse/AMBARI-18055
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> config page load takes long time on cluster with large number of config 
> versions
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
>  1484953 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceConfigVersionRequest.java
>  b32ccdf 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceConfigVersionResourceProvider.java
>  d7287e5 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ServiceConfigDAO.java
>  128fef0 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ServiceConfigEntity.java
>  22f82fc 
>   ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java 
> 0519123 
>   
> ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java
>  0d212a1 
>   
> ambari-server/src/test/java/org/apache/ambari/server/orm/dao/ServiceConfigDAOTest.java
>  78461dc 
>   ambari-web/app/utils/ajax/ajax.js e432e0c 
> 
> Diff: https://reviews.apache.org/r/50886/diff/
> 
> 
> Testing
> -------
> 
> Verifed manually on a deployed cluster
> Added a unit test. Verified that corresponding unit tests to the files that 
> have been changed passes.
> waiting for Hadoop QA job to post result
> 
> 
> Thanks,
> 
> Jaimin Jetly
> 
>

Reply via email to