> On May 13, 2016, 12:34 p.m., Jonathan Hurley wrote:
> > contrib/views/utils/src/main/java/org/apache/ambari/view/utils/UserLocal.java,
> >  lines 66-71
> > <https://reviews.apache.org/r/47305/diff/1/?file=1381317#file1381317line66>
> >
> >     Info level logging here could cause a lot of log statements.

I intentionally kept it info, to get the logs in case of some production issue 
which might not replicate when ambari-server restarted with debug mode. 
This will not create too many logs, it will create around number-of-hive-views 
* number-user-users using hive view, which should be fine.


> On May 13, 2016, 12:34 p.m., Jonathan Hurley wrote:
> > contrib/views/utils/src/main/java/org/apache/ambari/view/utils/UserLocal.java,
> >  lines 87-89
> > <https://reviews.apache.org/r/47305/diff/1/?file=1381317#file1381317line87>
> >
> >     Double-checked locking here; you'll need to make this volatile if 
> > you're keeping this code.

Do you mean to make viewSingletonObjects volatile? I don't think making 
viewSingletonObjects as volatile is required here as we are not changing the 
reference of viewSingletonObjects or assigning it new value. It is assigned 
once statically. And we are using contains method to check for existence of key 
inside that map. I would welcome any reason for making it volatile.


- Nitiraj


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


On May 12, 2016, 2:04 p.m., Nitiraj Rathore wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47305/
> -----------------------------------------------------------
> 
> (Updated May 12, 2016, 2:04 p.m.)
> 
> 
> Review request for Ambari, DIPAYAN BHOWMICK, Jonathan Hurley, Mahadev Konar, 
> Rohit Choudhary, and Ashwin Rajeev.
> 
> 
> Bugs: AMBARI-16146
>     https://issues.apache.org/jira/browse/AMBARI-16146
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Earlier : The synchronized UserLocalConnection.initialValue() was taking time 
> to get hive connection and all other threads were blocking to get inside this 
> synchronized block.
> 
> In this patch : 
> 1. Removed synchronized from UserLocalConnection.initialValue() and from 
> UserLocalHiveAuthCredentials.initialValue()
> 2. Introduced locking on specific key and not the complete method. Key would 
> be like Connection_INSTANCE-NAME:USER-NAME. So threads for other keys will 
> not get into contension with this lock. This avoids blocking of thread that 
> do not want to initialize different keys.
> 3. Used tryLock and fail with exception if lock cannot be obtained 
> immediately. This avoids blocking of threads that want to initialize same key 
> as one key should be initialized only once.
> 4. corrected synchronization at several places inside UserLocal class.
> 
> 
> Diffs
> -----
> 
>   
> contrib/views/hive/src/main/java/org/apache/ambari/view/hive/client/UserLocalConnection.java
>  c80a4c4 
>   
> contrib/views/hive/src/main/java/org/apache/ambari/view/hive/client/UserLocalHiveAuthCredentials.java
>  9c72863 
>   
> contrib/views/utils/src/main/java/org/apache/ambari/view/utils/UserLocal.java 
> 40c8e6e 
> 
> Diff: https://reviews.apache.org/r/47305/diff/
> 
> 
> Testing
> -------
> 
> Manual testing done by introducing Thread.sleep in 
> UserLocalConnection.initialValue() and creating more requests with following 
> combinations and logs.
> 0. first thread for connection + user + view combination obtains the lock and 
> goes inside UserLocalConnection.initialValue() and sleeps
> 1. same user, same hive view requests for connections fails to get lock with 
> exception.
> 2. same user, different hive view -- if first request gets new lock and goes 
> inside UserLocalConnection.initialValue().
> 3. same user, different hive view -- if connection created once then works 
> fine. does not block.
> 4. different user, same view -- gets new lock and goes inside 
> UserLocalConnection.initialValue() if first, else works fine.
> 5. different user, different view -- works as 4.
> 6. once the first thread come out of initialValue(), rest threads don't try 
> to get lock and don't fail either, works fine.
> 
> 
> Thanks,
> 
> Nitiraj Rathore
> 
>

Reply via email to