> 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088844#file2088844line3062> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088844#file2088844line3063> > > > > 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) > > <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.. 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line253> > > > > 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) > > <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 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line276> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line293> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line340> > > > > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line342> > > > > will there be a SessionState.getSessionConf() when this *enum* > > initializes? yes, enum will be initialized on first compile request. > On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java > > Lines 379 (patched) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line380> > > > > 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... renamed to getSessionLock() > 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... it's just a first steps in compile lock refactoring. > On Sept. 17, 2018, 9:15 a.m., Zoltan Haindrich wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java > > Lines 383 (patched) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line384> > > > > this method is a little bit confusing...getTimeout with a time > > argument; which seems to be maxTimeout value renamed to getRemainingTime() > 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) > > <https://reviews.apache.org/r/68683/diff/2/?file=2088845#file2088845line669> > > > > please don't make this method more visible; use compile("sel") or > > something...it should work 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. > 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.. 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. - denys ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68683/#review208625 ----------------------------------------------------------- On Sept. 17, 2018, 12:52 p.m., denys kuzmenko wrote: > > ----------------------------------------------------------- > 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 > ----- > > 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/ > > > 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 > >
