----------------------------------------------------------- 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) <https://reviews.apache.org/r/68683/#comment292792> 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) <https://reviews.apache.org/r/68683/#comment292794> 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) <https://reviews.apache.org/r/68683/#comment292723> 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) <https://reviews.apache.org/r/68683/#comment292732> final ql/src/java/org/apache/hadoop/hive/ql/Driver.java Lines 271 (patched) <https://reviews.apache.org/r/68683/#comment292724> 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) <https://reviews.apache.org/r/68683/#comment292733> 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) <https://reviews.apache.org/r/68683/#comment292725> 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) <https://reviews.apache.org/r/68683/#comment292726> is there any reason that this Lock is an enum? :) ql/src/java/org/apache/hadoop/hive/ql/Driver.java Lines 341 (patched) <https://reviews.apache.org/r/68683/#comment292795> will there be a SessionState.getSessionConf() when this *enum* initializes? ql/src/java/org/apache/hadoop/hive/ql/Driver.java Lines 379 (patched) <https://reviews.apache.org/r/68683/#comment292734> 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) <https://reviews.apache.org/r/68683/#comment292730> 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) <https://reviews.apache.org/r/68683/#comment292727> 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) <https://reviews.apache.org/r/68683/#comment292728> ql/src/java/org/apache/hadoop/hive/ql/Driver.java Line 507 (original), 666 (patched) <https://reviews.apache.org/r/68683/#comment292735> 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) <https://reviews.apache.org/r/68683/#comment292729> 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: > 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 > ----- > > 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/ > > > 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 > >
