> 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) > > <https://reviews.apache.org/r/68683/diff/5/?file=2090311#file2090311line130> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/5/?file=2090312#file2090312line1855> > > > > 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 > >
