You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergey Shelukhin <se...@hortonworks.com> on 2015/05/28 19:50:56 UTC
Review Request 34776: HIVE-4239 : Remove lock on compilation stage
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 49b8f97
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
ql/src/java/org/apache/hadoop/hive/ql/exec/DefaultFetchFormatter.java 37852ef
ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 20d0304
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86130
-----------------------------------------------------------
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java
<https://reviews.apache.org/r/34776/#comment138050>
this class is never used outside of HS2 codepath. so this isHiveServer2 check is not needed.
- Thejas Nair
On June 1, 2015, 8:25 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 8:25 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d22f11
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On June 2, 2015, 12:23 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java, line 103
> > <https://reviews.apache.org/r/34776/diff/4/?file=975858#file975858line103>
> >
> > would it be possible to use a synchronized set instead ? That would be less error prone.
> >
> >
> > Set<String> mySet = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
> >
> > I also see that we should be synchronizing on open/close in SessionManager and creation of new operations. But I think that is something for another jira since the primary goal of this one is not to fix issues when running multiple operations in a session.
close call take out all the elements in the set atomically (and clears the set); normal set doesn't support such operation. I don't know how necessary it is...
Given recent JDBC driver changes for multiple statements in one connection this may be a good change to make to prevent bugs, now that it's already done
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86129
-----------------------------------------------------------
On June 1, 2015, 8:25 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 8:25 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d22f11
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
> On June 2, 2015, 12:23 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java, line 103
> > <https://reviews.apache.org/r/34776/diff/4/?file=975858#file975858line103>
> >
> > would it be possible to use a synchronized set instead ? That would be less error prone.
> >
> >
> > Set<String> mySet = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
> >
> > I also see that we should be synchronizing on open/close in SessionManager and creation of new operations. But I think that is something for another jira since the primary goal of this one is not to fix issues when running multiple operations in a session.
>
> Sergey Shelukhin wrote:
> close call take out all the elements in the set atomically (and clears the set); normal set doesn't support such operation. I don't know how necessary it is...
> Given recent JDBC driver changes for multiple statements in one connection this may be a good change to make to prevent bugs, now that it's already done
I think the right way to do it is to synchronize on open/close vs creation of new operations in SessionManager. When the session is being closed, it should be within a lock and first thing it should do is to remove the Sessionhandle from the map so that no other operation can access it.
It takes courage to delete old code, specially if they are doing some synchronization , I think it is better to do it the better way in a follow up jira.
The JDBC driver changes does not suddenly enable use of multiple statements in one connection. That has always been there, that patch just adds some locks on it. So I think it is OK to do that as part of new jira.
- Thejas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86129
-----------------------------------------------------------
On June 2, 2015, 12:53 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 12:53 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86129
-----------------------------------------------------------
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
<https://reviews.apache.org/r/34776/#comment138049>
would it be possible to use a synchronized set instead ? That would be less error prone.
Set<String> mySet = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
I also see that we should be synchronizing on open/close in SessionManager and creation of new operations. But I think that is something for another jira since the primary goal of this one is not to fix issues when running multiple operations in a session.
- Thejas Nair
On June 1, 2015, 8:25 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 8:25 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d22f11
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
(Updated June 15, 2015, 11:03 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 27f68df
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 669e6be
ql/src/java/org/apache/hadoop/hive/ql/optimizer/RemoveDynamicPruningBySize.java 5d01311
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java adc31ae
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 11c1df6
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezWork.java 6db8220
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 56707af
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37d856c
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java a2fae69
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java 56af643
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On June 4, 2015, 10:31 p.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java, line 184
> > <https://reviews.apache.org/r/34776/diff/5-6/?file=976065#file976065line184>
> >
> > shoudl this also be static ?
no, see class comment - tests override it
> On June 4, 2015, 10:31 p.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java, line 209
> > <https://reviews.apache.org/r/34776/diff/5-6/?file=976065#file976065line209>
> >
> > also make static ?
same
> On June 4, 2015, 10:31 p.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java, line 321
> > <https://reviews.apache.org/r/34776/diff/6/?file=976761#file976761line321>
> >
> > can't everything in the GenTezUtils be made static and then we don't need to create any instances of it ?
> > Maybe make the constructor of GenTezUtils private.
see above?
> On June 4, 2015, 10:31 p.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java, line 55
> > <https://reviews.apache.org/r/34776/diff/5/?file=976068#file976068line55>
> >
> > HiveSessionProxy is the one that does the doAs().
> >
> > I looked some more, it looks like this should not cause a problem with the safeguards added in HIVE-7890. (but I think we should treat that change as a safeguard, as I am not sure of the performance implications of that additional check every time).
> >
> > However, with this approach, we would end up creating a new Hive object in most cases, if there are large number of users. Also the checks done in "Hive.get(conf);" to see if new object needs to be created is not trivial.
> >
> > The cases were sessions are used in parallel is rare, so paying this performance penalty for the more common case is not really justified IMO.
> >
> > I think we should just use locks around metastore client class for that. I think thats something we should pursue in a different jira.
does it require anything in this JIRA?
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86727
-----------------------------------------------------------
On June 2, 2015, 7:07 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:07 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/RemoveDynamicPruningBySize.java 5d01311
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java adc31ae
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezWork.java 6db8220
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 56707af
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
> On June 4, 2015, 10:31 p.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java, line 55
> > <https://reviews.apache.org/r/34776/diff/5/?file=976068#file976068line55>
> >
> > HiveSessionProxy is the one that does the doAs().
> >
> > I looked some more, it looks like this should not cause a problem with the safeguards added in HIVE-7890. (but I think we should treat that change as a safeguard, as I am not sure of the performance implications of that additional check every time).
> >
> > However, with this approach, we would end up creating a new Hive object in most cases, if there are large number of users. Also the checks done in "Hive.get(conf);" to see if new object needs to be created is not trivial.
> >
> > The cases were sessions are used in parallel is rare, so paying this performance penalty for the more common case is not really justified IMO.
> >
> > I think we should just use locks around metastore client class for that. I think thats something we should pursue in a different jira.
>
> Sergey Shelukhin wrote:
> does it require anything in this JIRA?
yes, remove the changes to HiveSessionImplwithUGI.java from this jira.
- Thejas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86727
-----------------------------------------------------------
On June 2, 2015, 7:07 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:07 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/RemoveDynamicPruningBySize.java 5d01311
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java adc31ae
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezWork.java 6db8220
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 56707af
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86727
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java
<https://reviews.apache.org/r/34776/#comment138788>
shoudl this also be static ?
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java
<https://reviews.apache.org/r/34776/#comment138789>
also make static ?
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java
<https://reviews.apache.org/r/34776/#comment138791>
HiveSessionProxy is the one that does the doAs().
I looked some more, it looks like this should not cause a problem with the safeguards added in HIVE-7890. (but I think we should treat that change as a safeguard, as I am not sure of the performance implications of that additional check every time).
However, with this approach, we would end up creating a new Hive object in most cases, if there are large number of users. Also the checks done in "Hive.get(conf);" to see if new object needs to be created is not trivial.
The cases were sessions are used in parallel is rare, so paying this performance penalty for the more common case is not really justified IMO.
I think we should just use locks around metastore client class for that. I think thats something we should pursue in a different jira.
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java
<https://reviews.apache.org/r/34776/#comment138792>
Thanks for cleaning up the utils and threadlocal use!
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java
<https://reviews.apache.org/r/34776/#comment138790>
can't everything in the GenTezUtils be made static and then we don't need to create any instances of it ?
Maybe make the constructor of GenTezUtils private.
- Thejas Nair
On June 2, 2015, 7:07 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:07 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/RemoveDynamicPruningBySize.java 5d01311
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java adc31ae
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezWork.java 6db8220
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 56707af
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
(Updated June 2, 2015, 7:07 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
ql/src/java/org/apache/hadoop/hive/ql/optimizer/RemoveDynamicPruningBySize.java 5d01311
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java adc31ae
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezWork.java 6db8220
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 56707af
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On June 2, 2015, 2:37 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java, line 55
> > <https://reviews.apache.org/r/34776/diff/5/?file=976068#file976068line55>
> >
> > I think this approach will cause the bug in HIVE-6245.
> > If new Hive object is not created within doAs block, it would not create it with the correct user.
> > I need to look some more into that.
Can you point at the doAs block you have in mind in existing code? ctor is called from openSession and that from variety of openSession overloads in the service, with no doAs blocks.
I can replicate it in createHive if needed
> On June 2, 2015, 2:37 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java, line 74
> > <https://reviews.apache.org/r/34776/diff/5/?file=976065#file976065line74>
> >
> > If we follow this approach every place, it is going to lead to an explosion of thread local.
> >
> > Also, the use of thread local this way can lead to new bugs getting introduced over time.
> >
> > The driver objects lifetime is beyond compilation. The execution is usually done in a different thread.
> >
> > I feel a cleaner/more robust solution would be to have these objects tied to a driver instance rather than a thread. Not sure what the best approach for that would be - maybe have a thread local driver, and have it contain a context object that holds related fields ? Thoughts ?
> >
> > Rant : Having a Utils class have state is also counter intuitive, maybe the part that had state should have been part of a different class ..
There's a separate jira to refactor and add OperationState. I got rid of utils threadlocal; I think having one per session would be safe enough. All the required fields can be put in one threadlocal.
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86144
-----------------------------------------------------------
On June 2, 2015, 12:53 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 12:53 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86144
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java
<https://reviews.apache.org/r/34776/#comment138067>
If we follow this approach every place, it is going to lead to an explosion of thread local.
Also, the use of thread local this way can lead to new bugs getting introduced over time.
The driver objects lifetime is beyond compilation. The execution is usually done in a different thread.
I feel a cleaner/more robust solution would be to have these objects tied to a driver instance rather than a thread. Not sure what the best approach for that would be - maybe have a thread local driver, and have it contain a context object that holds related fields ? Thoughts ?
Rant : Having a Utils class have state is also counter intuitive, maybe the part that had state should have been part of a different class ..
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java
<https://reviews.apache.org/r/34776/#comment138071>
I think this approach will cause the bug in HIVE-6245.
If new Hive object is not created within doAs block, it would not create it with the correct user.
I need to look some more into that.
service/src/java/org/apache/hive/service/server/HiveServer2.java
<https://reviews.apache.org/r/34776/#comment138072>
why do you still need this ?
- Thejas Nair
On June 2, 2015, 12:53 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 12:53 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
(Updated June 2, 2015, 12:53 a.m.)
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
(Updated June 1, 2015, 8:25 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d22f11
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
(Updated May 29, 2015, 7:56 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/util/QFileClient.java b62a883
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 49b8f97
itests/qtest/pom.xml e90f6f4
itests/src/test/resources/testconfiguration.properties bdb96e8
pom.xml b21d894
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
ql/src/java/org/apache/hadoop/hive/ql/exec/DefaultFetchFormatter.java 37852ef
ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 20d0304
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
ql/src/test/templates/TestBeeLineDriver.vm 563d7fd
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
testutils/ptest/hivetest.py ccc4537
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Thejas Nair <th...@hortonworks.com>.
> On May 29, 2015, 10:34 a.m., Carl Steinbach wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1133
> > <https://reviews.apache.org/r/34776/diff/2/?file=973800#file973800line1133>
> >
> > This:
> >
> > if (isParallelEnabled && LOG.isDebugEnabled()) {
> > LOG.debug("Entering compile: " + command);
> > }
> >
> > is slower than this:
> >
> > if (isParallelEnabled){
> > LOG.debug("Entering compile: " + command);
> > }
> >
> > Ref: http://stackoverflow.com/a/963681
That stack overflow reference is about case where the log message is just a constant string, I agree that checking isDebugEnabled is not a great idea in that case.
In this case, two strings are being appended. Checking the boolean is likely cheaper than creating the new string object.
- Thejas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review85703
-----------------------------------------------------------
On May 28, 2015, 9:31 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 9:31 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 49b8f97
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/exec/DefaultFetchFormatter.java 37852ef
> ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 20d0304
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Carl Steinbach <cw...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review85703
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/Driver.java
<https://reviews.apache.org/r/34776/#comment137438>
This:
if (isParallelEnabled && LOG.isDebugEnabled()) {
LOG.debug("Entering compile: " + command);
}
is slower than this:
if (isParallelEnabled){
LOG.debug("Entering compile: " + command);
}
Ref: http://stackoverflow.com/a/963681
- Carl Steinbach
On May 28, 2015, 9:31 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 9:31 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 49b8f97
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
> ql/src/java/org/apache/hadoop/hive/ql/exec/DefaultFetchFormatter.java 37852ef
> ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 20d0304
> ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
> service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
> service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
>
> Diff: https://reviews.apache.org/r/34776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 34776: HIVE-4239 : Remove lock on compilation stage
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/
-----------------------------------------------------------
(Updated May 28, 2015, 9:31 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 49b8f97
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f
ql/src/java/org/apache/hadoop/hive/ql/exec/DefaultFetchFormatter.java 37852ef
ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 20d0304
ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1
service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49
service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f
Diff: https://reviews.apache.org/r/34776/diff/
Testing
-------
Thanks,
Sergey Shelukhin