> 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line248> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line253> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line272> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line381> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line2022> > > > > 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 > > 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 > >
