----------------------------------------------------------- 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) <https://reviews.apache.org/r/68683/#comment293234> 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) <https://reviews.apache.org/r/68683/#comment293239> 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 > >
