Re: Review Request 50886: config page load takes long time on cluster with large number of config versions

2016-08-10 Thread Jonathan Hurley 

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


Ship it!




Ship It!

- Jonathan Hurley


On Aug. 9, 2016, 6:45 p.m., Jaimin Jetly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50886/
> ---
> 
> (Updated Aug. 9, 2016, 6:45 p.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
> 
>

Re: Review Request 50886: config page load takes long time on cluster with large number of config versions

2016-08-09 Thread Jaimin Jetly 


> 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
> > 
> >
> > 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?
> 
> Jaimin Jetly wrote:
> 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.

created https://issues.apache.org/jira/browse/AMBARI-18090 to track 
optimization of 
http://c6401.ambari.apache.org:8080/api/v1/clusters/c1/configurations/service_config_versions?is_current=true&fields=*
 API. I have kept fixed verion as trunk since this API s not being called by 
ambari-web and so does not have an impact when using ambari web client


- Jaimin


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


On Aug. 9, 2016, 10:45 p.m., Jaimin Jetly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50886/
> ---
> 
> (Updated Aug. 9, 2016, 10:45 p.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
> 
>

Re: Review Request 50886: config page load takes long time on cluster with large number of config versions

2016-08-09 Thread Jaimin Jetly 

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

(Updated Aug. 9, 2016, 10:45 p.m.)


Review request for Ambari, Dmytro Sen, Jonathan Hurley, Myroslav Papirkovskyy, 
Sumit Mohanty, and Sid Wagle.


Changes
---

2nd revision of the patch addresses concern raised on reviewboard


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 (updated)
-

  
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

Re: Review Request 50886: config page load takes long time on cluster with large number of config versions

2016-08-09 Thread Jaimin Jetly 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 50886: config page load takes long time on cluster with large number of config versions

2016-08-09 Thread Jonathan Hurley 

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




ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
 (line 21)


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



ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
 (lines 3519 - 3526)


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?



ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ServiceConfigEntity.java
 (lines 53 - 54)


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.



ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java
 (lines 2707 - 2709)


Why not use the simpler constructor for ConfigurationResponse which takes a 
Config instance?


- Jonathan Hurley


On Aug. 8, 2016, 3: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, 3: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
> 
>