Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Oct. 1, 2018, 11:42 a.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, and 
Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d1e6631975 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java dad2035362 
  ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/8/

Changes: https://reviews.apache.org/r/68683/diff/7-8/


Testing
---

Added CompileLockTest


File Attachments


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch


Thanks,

denys kuzmenko

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[Peter Vary via Review Board]

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



Thanks Denys,
I like this new version.
My last comments are below.
What do you think is it worth to create a new version of the patch?
Thanks,
Peter


ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java
Lines 44 (patched)


Would it be a good idea to remove the public constructor? We are using 
factory to create CompileLock, so we might want to emphasize that



ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java
Lines 64 (patched)


We do not use this anywhere - we might want to consider to remove this 
altogether and keep only the one without parameters?



ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java
Lines 110 (patched)


Can this cause problems if called after a failed tryAcquire? Since this 
method is not used anywhere outside this class, it might be a good idea to 
merge with close.


- Peter Vary


On szept. 26, 2018, 1:08 du, denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated szept. 26, 2018, 1:08 du)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/7/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

> On Sept. 26, 2018, 11:47 a.m., Antal Sinkovits wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
> > Lines 3062 (patched)
> > 
> >
> > Why is the default value -1? All the checks seems to go against >0. 
> > What happens when the value is 0?

If the degree of parallelism is lower than 1, there won't be any restrictions 
on number of parallel compilations. "-1" means unbounded. "0" doesn't make 
sence, so it's just ignored (and will be unbounded).  Otherwise, number of 
quotas will be equal to the "hive.driver.parallel.compilation.global.limit" 
value.


> On Sept. 26, 2018, 11:47 a.m., Antal Sinkovits wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java
> > Lines 12 (patched)
> > 
> >
> > I think there is a typo here, and the it should be CompileLock.class.

Thanks for the catch!


- denys


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


On Sept. 26, 2018, 1:08 p.m., denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated Sept. 26, 2018, 1:08 p.m.)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/7/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Sept. 26, 2018, 1:08 p.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, and 
Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
  ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/7/

Changes: https://reviews.apache.org/r/68683/diff/6-7/


Testing
---

Added CompileLockTest


File Attachments


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch


Thanks,

denys kuzmenko

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[Antal Sinkovits via Review Board]

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




common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
Lines 3062 (patched)


Why is the default value -1? All the checks seems to go against >0. What 
happens when the value is 0?



ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java
Lines 12 (patched)


I think there is a typo here, and the it should be CompileLock.class.


- Antal Sinkovits


On szept. 25, 2018, 10:19 de, denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated szept. 25, 2018, 10:19 de)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/6/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

> On Sept. 24, 2018, 11:14 p.m., Peter Vary wrote:
> > Hi Denys,
> > 
> > Could you please think a little about separating the Manager/Factory and 
> > the tryAcquire mess?
> > 
> > Incomplete thoughts, but I had to run
> > 
> > Thanks, and sorry :(
> > Peter

Please review new patch. Really appreciate your and Zoltan's suggestions, now 
code looks much better.


> On Sept. 24, 2018, 11:14 p.m., Peter Vary wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java
> > Lines 130 (patched)
> > 
> >
> > nit: I do prefer creating static final variables at the begining of the 
> > class, or at the first use. Do not create a new patch because of this, but 
> > if you have to do a new one please move the declaration up to the line ~51

done!


> On Sept. 24, 2018, 11:14 p.m., Peter Vary wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Line 1854 (original), 1849-1850 (patched)
> > 
> >
> > This still makes me itching...
> > I think we should separate the Manager / Factory and the actual lock 
> > object.
> > I would prefer the following:
> > - CompileLockManager should create the lock object
> > - Use the lock object as Zoltan suggested (try-with-resources)
> > - If we decide to keep tryAcquire - can we do it as a wrapper around 
> > the tryLock method

Please review new path! Thank you!


- denys


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


On Sept. 25, 2018, 10:19 a.m., denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated Sept. 25, 2018, 10:19 a.m.)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/6/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
> HIVE-20535.14.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > I'm not sure but I feel that it would be probably simpler to add something 
> > which covers some reentrant-s and semaphores.
> > It feels like this lock handling is a littlebit scattered around...I think 
> > it would be better to have them outside of the Driver class.
> 
> denys kuzmenko wrote:
> moved logic to CompileLockManager

splitted and extracted functionality of CompileLock and CompileLockFactory


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 247 (patched)
> > 
> >
> > I'm not sure we gain anything by having these strings in a static block 
> > - they are only used as log messages at debug level..
> 
> denys kuzmenko wrote:
> It's a clean code practice (String literals)

refactored


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 252 (patched)
> > 
> >
> > final
> 
> denys kuzmenko wrote:
> there is conditional logic, default value is serializableCompileLock;

Fixed.


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 271 (patched)
> > 
> >
> > it seems to me that this class is not the lock itself...it instead the 
> > "thing that locks"...
> > 
> > but getInstance() gives the feeling that it's something like a 
> > singleton...this is a little bit confusing to me
> 
> denys kuzmenko wrote:
> externalized to CompileLockManager class

refactored, added Factory and Lock


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 380 (patched)
> > 
> >
> > my first comment was: why do we use 2 locks now?
> > 
> > I now understand why...I feel that probably trying to replace the 
> > existing logic with a decent one which could handle all these cases would 
> > make it more straight.
> > If you don't think that would be appropriate - that's okay; just drop 
> > this issue...
> 
> denys kuzmenko wrote:
> it's just a first steps in compile lock refactoring.

New locks could be created with CompileLockFactory.


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Line 1860 (original), 2017 (patched)
> > 
> >
> > I think it would be better to use try-with-resouces instead of manual 
> > control...that would also take care of the unlock/release/etc as well
> > 
> > I feel that it's easier to follow - if a lock has a scope..
> 
> denys kuzmenko wrote:
> I would have to remember the result of tryAcquire method (aquired lock or 
> not) and supply it to AutoClosable.close(){if(locked) lock.unlock()} . I 
> think it would complicate the logic.

Fixed


- denys


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


On Sept. 25, 2018, 10:19 a.m., denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated Sept. 25, 2018, 10:19 a.m.)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/6/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> HIVE-20535.14.patch
>   
> 

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Sept. 25, 2018, 10:19 a.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, and 
Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
  ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLock.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/lock/CompileLockFactory.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/6/

Changes: https://reviews.apache.org/r/68683/diff/5-6/


Testing
---

Added CompileLockTest


File Attachments (updated)


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/335b0f4b-ea94-41d4-881a-ec8bb870a376__HIVE-20535.14.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/a92b6da2-eeba-46ee-9409-162653826172__HIVE-20535.14.patch
HIVE-20535.14.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/25/9db4cf76-9188-48fb-bd3d-5b28e43a791b__HIVE-20535.14.patch


Thanks,

denys kuzmenko

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[Peter Vary via Review Board]

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



Hi Denys,

Could you please think a little about separating the Manager/Factory and the 
tryAcquire mess?

Incomplete thoughts, but I had to run

Thanks, and sorry :(
Peter


ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java
Lines 130 (patched)


nit: I do prefer creating static final variables at the begining of the 
class, or at the first use. Do not create a new patch because of this, but if 
you have to do a new one please move the declaration up to the line ~51



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Line 1854 (original), 1849-1850 (patched)


This still makes me itching...
I think we should separate the Manager / Factory and the actual lock object.
I would prefer the following:
- CompileLockManager should create the lock object
- Use the lock object as Zoltan suggested (try-with-resources)
- If we decide to keep tryAcquire - can we do it as a wrapper around the 
tryLock method


- Peter Vary


On szept. 19, 2018, 9:37 de, denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated szept. 19, 2018, 9:37 de)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/5/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Line 507 (original), 666 (patched)
> > 
> >
> > please don't make this method more visible; use compile("sel") or 
> > something...it should work
> 
> denys kuzmenko wrote:
> it's impossible to mock and test compile lock behaviour. Entry point is 
> Driver.compileAndRespond("query"). I do not want to use PowerMock. Actually I 
> tried and faced many issues with hadoop classes.
> 
> Peter Vary wrote:
> What about @VisibleForTesting annotation? It could show the intention at 
> least...

Added @VisibleForTesting annotation


- denys


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


On Sept. 19, 2018, 9:37 a.m., denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated Sept. 19, 2018, 9:37 a.m.)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
>   ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/5/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Sept. 19, 2018, 9:37 a.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, and 
Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
  ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/5/

Changes: https://reviews.apache.org/r/68683/diff/4-5/


Testing
---

Added CompileLockTest


File Attachments


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch


Thanks,

denys kuzmenko

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[Peter Vary via Review Board]

> On szept. 17, 2018, 9:15 de, Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Line 507 (original), 666 (patched)
> > 
> >
> > please don't make this method more visible; use compile("sel") or 
> > something...it should work
> 
> denys kuzmenko wrote:
> it's impossible to mock and test compile lock behaviour. Entry point is 
> Driver.compileAndRespond("query"). I do not want to use PowerMock. Actually I 
> tried and faced many issues with hadoop classes.

What about @VisibleForTesting annotation? It could show the intention at 
least...


- Peter


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


On szept. 17, 2018, 5:55 du, denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68683/
> ---
> 
> (Updated szept. 17, 2018, 5:55 du)
> 
> 
> Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, 
> and Peter Vary.
> 
> 
> Bugs: HIVE-20535
> https://issues.apache.org/jira/browse/HIVE-20535
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> When removing the compile lock, it is quite risky to remove it entirely.
> 
> It would be good to provide a pool size for the concurrent compilation, so 
> the administrator can limit the load
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3fb8e76 
>   ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java dad2035 
>   ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68683/diff/4/
> 
> 
> Testing
> ---
> 
> Added CompileLockTest
> 
> 
> File Attachments
> 
> 
> HIVE-20535.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch
> 
> 
> Thanks,
> 
> denys kuzmenko
> 
>

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Sept. 17, 2018, 5:55 p.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, and 
Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3fb8e76 
  ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java dad2035 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/4/

Changes: https://reviews.apache.org/r/68683/diff/3-4/


Testing
---

Added CompileLockTest


File Attachments


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch


Thanks,

denys kuzmenko

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > I'm not sure but I feel that it would be probably simpler to add something 
> > which covers some reentrant-s and semaphores.
> > It feels like this lock handling is a littlebit scattered around...I think 
> > it would be better to have them outside of the Driver class.

moved logic to CompileLockManager


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
> > Line 3062 (original), 3062 (patched)
> > 
> >
> > I think we don't want to start reinterpreting an existing option
> > 
> > consider adding:
> > 
> > * session level compilation limit
> > * global compilation limit
> > 
> > 
> > right now it seems to me that the "within the same session" is not 
> > possible right now...; because it acquires the sessionstate reentrant 
> > during quota checks

"within the same session" option is not possible right now, that is why I 
removed it from the property description as it was misleading.
Currently session level compilation limit is always 1. 

At this moment, we wanted to introduce just a global compilation limit - not to 
overwhelm the cluster. As of now, we don't have any restrictions for the number 
of compilation quaries in case parralel compilation option is set to true.

reverted.


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
> > Lines 3063 (patched)
> > 
> >
> > If we want to do session level parallelism; I think this should be 
> > renamed to:
> > 
> > hive.driver.parallel.compilation.global.limit

OK


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 247 (patched)
> > 
> >
> > I'm not sure we gain anything by having these strings in a static block 
> > - they are only used as log messages at debug level..

It's a clean code practice (String literals)


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 252 (patched)
> > 
> >
> > final

there is conditional logic, default value is serializableCompileLock;


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 271 (patched)
> > 
> >
> > it seems to me that this class is not the lock itself...it instead the 
> > "thing that locks"...
> > 
> > but getInstance() gives the feeling that it's something like a 
> > singleton...this is a little bit confusing to me

externalized to CompileLockManager class


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 275 (patched)
> > 
> >
> > intention/use mismatch:
> > 
> > this method is private; however it seems to be called from compile()

changed to public


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 292 (patched)
> > 
> >
> > I think we don't need this "try first"
> > 
> > java.util.concurrent.locks.AbstractQueuedSynchronizer#tryAcquireNanos 
> > seems to be already doing this trick...
> > 
> > ```
> > return tryAcquire(arg) ||
> > doAcquireNanos(arg, nanosTimeout);
> > ```
> > 
> > ...or there are some other benefits?

Completely agree with this!!! I would remove this "try first" in first place, 
however I noticed that this logic was introduced intentionally: 
HIVE-14263 : Log message when HS2 query is waiting on compile lock (Tao Li via 
Thejas Nair)

Not sure how important is that and whether we can remove it???


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 339 (patched)
> > 
> >
> > is there any reason that this Lock is an enum? :)

It's a more cleaner way to implement a singleton, than having double-ckecked 
locking. This class initiatlizes a global semaphore used to controll compile 
limit.


> On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 341 (patched)
> > 
> >
> > will there be a 

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Sept. 17, 2018, 12:52 p.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, Naveen Gangam, and 
Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
  ql/src/java/org/apache/hadoop/hive/ql/CompileLockManager.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/3/

Changes: https://reviews.apache.org/r/68683/diff/2-3/


Testing
---

Added CompileLockTest


File Attachments


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch


Thanks,

denys kuzmenko

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[Zoltan Haindrich]

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



I'm not sure but I feel that it would be probably simpler to add something 
which covers some reentrant-s and semaphores.
It feels like this lock handling is a littlebit scattered around...I think it 
would be better to have them outside of the Driver class.


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
Line 3062 (original), 3062 (patched)


I think we don't want to start reinterpreting an existing option

consider adding:

* session level compilation limit
* global compilation limit

right now it seems to me that the "within the same session" is not possible 
right now...; because it acquires the sessionstate reentrant during quota checks



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
Lines 3063 (patched)


If we want to do session level parallelism; I think this should be renamed 
to:

hive.driver.parallel.compilation.global.limit



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 247 (patched)


I'm not sure we gain anything by having these strings in a static block - 
they are only used as log messages at debug level..



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 252 (patched)


final



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 271 (patched)


it seems to me that this class is not the lock itself...it instead the 
"thing that locks"...

but getInstance() gives the feeling that it's something like a 
singleton...this is a little bit confusing to me



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 275 (patched)


intention/use mismatch:

this method is private; however it seems to be called from compile()



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 292 (patched)


I think we don't need this "try first"

java.util.concurrent.locks.AbstractQueuedSynchronizer#tryAcquireNanos seems 
to be already doing this trick...

```
return tryAcquire(arg) ||
doAcquireNanos(arg, nanosTimeout);
```

...or there are some other benefits?



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 339 (patched)


is there any reason that this Lock is an enum? :)



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 341 (patched)


will there be a SessionState.getSessionConf() when this *enum* initializes?



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 379 (patched)


this method name is ambigous - I don't know what to expect...

* this class is a "compileLock"
* there is one in SessionState
* and there is the the new CompileLock inner class...



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 380 (patched)


my first comment was: why do we use 2 locks now?

I now understand why...I feel that probably trying to replace the existing 
logic with a decent one which could handle all these cases would make it more 
straight.
If you don't think that would be appropriate - that's okay; just drop this 
issue...



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 383 (patched)


this method is a little bit confusing...getTimeout with a time argument; 
which seems to be maxTimeout value



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 384 (patched)






ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Line 507 (original), 666 (patched)


please don't make this method more visible; use compile("sel") or 
something...it should work



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Line 1860 (original), 2017 (patched)


I think it would be better to use try-with-resouces instead of manual 
control...that would also take care of the unlock/release/etc as well

I feel that it's easier to follow - if a lock has a scope..


- Zoltan Haindrich


On Sept. 13, 2018, 8:18 p.m., denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 

Re: Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

(Updated Sept. 13, 2018, 8:18 p.m.)


Review request for hive, Zoltan Haindrich, Zoltan Haindrich, and Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java aa58d74 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java dad2035 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/2/

Changes: https://reviews.apache.org/r/68683/diff/1-2/


Testing
---

Added CompileLockTest


File Attachments (updated)


HIVE-20535.1.patch
  
https://reviews.apache.org/media/uploaded/files/2018/09/13/41f5a84a-70e5-4882-99c1-1cf98c4364e4__HIVE-20535.1.patch


Thanks,

denys kuzmenko

Review Request 68683: Add new configuration to set the size of the global compile lock

[denys kuzmenko via Review Board]

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

Review request for hive, Zoltan Haindrich, Zoltan Haindrich, and Peter Vary.


Bugs: HIVE-20535
https://issues.apache.org/jira/browse/HIVE-20535


Repository: hive-git


Description
---

When removing the compile lock, it is quite risky to remove it entirely.

It would be good to provide a pool size for the concurrent compilation, so the 
administrator can limit the load


Diffs
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java
 af0f87bac3 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8c39de3e77 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 737debd2ad 
  ql/src/test/org/apache/hadoop/hive/ql/CompileLockTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/68683/diff/1/


Testing
---

Added CompileLockTest


Thanks,

denys kuzmenko