You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2013/10/08 03:29:58 UTC
Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-5486
https://issues.apache.org/jira/browse/HIVE-5486
Repository: hive-git
Description
-------
With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
The patch is creating base scratch dirs at startup if it doesn't exist.
Diffs
-----
service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
Diff: https://reviews.apache.org/r/14523/diff/
Testing
-------
Added new test
Thanks,
Prasad Mujumdar
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On Oct. 8, 2013, 9:30 p.m., Brock Noland wrote:
> > Hi Prasad,
> >
> > I think this is a good change. There is one item below that might need resolving?
Thanks Brock!
Addressed the comments will update the patch
> On Oct. 8, 2013, 9:30 p.m., Brock Noland wrote:
> > service/src/java/org/apache/hive/service/cli/CLIService.java, line 85
> > <https://reviews.apache.org/r/14523/diff/2/?file=362071#file362071line85>
> >
> > userVars contains
> >
> > /**
> > * The conf variables that depends on current user
> > */
> > public static final HiveConf.ConfVars[] userVars = {
> > HiveConf.ConfVars.SCRATCHDIR,
> > HiveConf.ConfVars.LOCALSCRATCHDIR,
> > HiveConf.ConfVars.DOWNLOADED_RESOURCES_DIR,
> > HiveConf.ConfVars.HIVEHISTORYFILELOC
> > };
> >
> >
> > Do the other two need to be configured as well?
yes, I think DOWNLOADED_RESOURCES_DIR should be created. The hive history is disabled by default and not useful for HiveServer2. That should be left out. Thanks for catching that.
> On Oct. 8, 2013, 9:30 p.m., Brock Noland wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java, line 142
> > <https://reviews.apache.org/r/14523/diff/2/?file=362072#file362072line142>
> >
> > userVars is unused after this change.
right. Removed
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/#review26797
-----------------------------------------------------------
On Oct. 8, 2013, 1:34 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14523/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2013, 1:34 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-5486
> https://issues.apache.org/jira/browse/HIVE-5486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
> The patch is creating base scratch dirs at startup if it doesn't exist.
>
>
> Diffs
> -----
>
> service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
> service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
> service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14523/diff/
>
>
> Testing
> -------
>
> Added new test
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/#review26797
-----------------------------------------------------------
Hi Prasad,
I think this is a good change. There is one item below that might need resolving?
service/src/java/org/apache/hive/service/cli/CLIService.java
<https://reviews.apache.org/r/14523/#comment52139>
userVars contains
/**
* The conf variables that depends on current user
*/
public static final HiveConf.ConfVars[] userVars = {
HiveConf.ConfVars.SCRATCHDIR,
HiveConf.ConfVars.LOCALSCRATCHDIR,
HiveConf.ConfVars.DOWNLOADED_RESOURCES_DIR,
HiveConf.ConfVars.HIVEHISTORYFILELOC
};
Do the other two need to be configured as well?
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java
<https://reviews.apache.org/r/14523/#comment52140>
userVars is unused after this change.
- Brock Noland
On Oct. 8, 2013, 1:34 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14523/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2013, 1:34 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-5486
> https://issues.apache.org/jira/browse/HIVE-5486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
> The patch is creating base scratch dirs at startup if it doesn't exist.
>
>
> Diffs
> -----
>
> service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
> service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
> service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14523/diff/
>
>
> Testing
> -------
>
> Added new test
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/#review26820
-----------------------------------------------------------
Ship it!
Ship It!
- Brock Noland
On Oct. 9, 2013, 6:34 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14523/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2013, 6:34 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-5486
> https://issues.apache.org/jira/browse/HIVE-5486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
> The patch is creating base scratch dirs at startup if it doesn't exist.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d0895e1
> service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
> service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
> service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14523/diff/
>
>
> Testing
> -------
>
> Added new test
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On Oct. 17, 2013, 2:05 a.m., Thejas Nair wrote:
> >
Addressed the suggestions. will rebase and upload the new patch.
> On Oct. 17, 2013, 2:05 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/CLIService.java, line 94
> > <https://reviews.apache.org/r/14523/diff/3/?file=362770#file362770line94>
> >
> > This can lead to a misleading error message. I think it is better to have two try-catch blocks.
> >
Make sense. Done.
> On Oct. 17, 2013, 2:05 a.m., Thejas Nair wrote:
> > service/src/test/org/apache/hive/service/cli/TestScratchDir.java, line 37
> > <https://reviews.apache.org/r/14523/diff/3/?file=362773#file362773line37>
> >
> > Thanks for adding a test case!
> >
> > Can you change "/tmp" to System.getProperty("test.tmp.dir"); ?
> >
> > Even better would be to also rename "foobar" to "TestScratchDirs_foobar"
> >
> > It looks straightforward to add check for other two configured dirs. Can you include them as well ?
Done.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/#review27105
-----------------------------------------------------------
On Oct. 9, 2013, 6:34 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14523/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2013, 6:34 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-5486
> https://issues.apache.org/jira/browse/HIVE-5486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
> The patch is creating base scratch dirs at startup if it doesn't exist.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d0895e1
> service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
> service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
> service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14523/diff/
>
>
> Testing
> -------
>
> Added new test
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/#review27105
-----------------------------------------------------------
service/src/java/org/apache/hive/service/cli/CLIService.java
<https://reviews.apache.org/r/14523/#comment52763>
This can lead to a misleading error message. I think it is better to have two try-catch blocks.
service/src/test/org/apache/hive/service/cli/TestScratchDir.java
<https://reviews.apache.org/r/14523/#comment52765>
Thanks for adding a test case!
Can you change "/tmp" to System.getProperty("test.tmp.dir"); ?
Even better would be to also rename "foobar" to "TestScratchDirs_foobar"
It looks straightforward to add check for other two configured dirs. Can you include them as well ?
- Thejas Nair
On Oct. 9, 2013, 6:34 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14523/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2013, 6:34 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-5486
> https://issues.apache.org/jira/browse/HIVE-5486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
> The patch is creating base scratch dirs at startup if it doesn't exist.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d0895e1
> service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
> service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
> service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14523/diff/
>
>
> Testing
> -------
>
> Added new test
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/
-----------------------------------------------------------
(Updated Oct. 19, 2013, 6:50 p.m.)
Review request for hive.
Changes
-------
Updated patch per review feedback
Bugs: HIVE-5486
https://issues.apache.org/jira/browse/HIVE-5486
Repository: hive-git
Description
-------
With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
The patch is creating base scratch dirs at startup if it doesn't exist.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java fb3570b
service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
Diff: https://reviews.apache.org/r/14523/diff/
Testing
-------
Added new test
Thanks,
Prasad Mujumdar
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/#review26812
-----------------------------------------------------------
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java
<https://reviews.apache.org/r/14523/#comment52169>
yes. Removed.
- Prasad Mujumdar
On Oct. 9, 2013, 6:34 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14523/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2013, 6:34 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-5486
> https://issues.apache.org/jira/browse/HIVE-5486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
> The patch is creating base scratch dirs at startup if it doesn't exist.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d0895e1
> service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
> service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
> service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14523/diff/
>
>
> Testing
> -------
>
> Added new test
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/
-----------------------------------------------------------
(Updated Oct. 9, 2013, 6:34 a.m.)
Review request for hive.
Changes
-------
Changes per review feedback
Bugs: HIVE-5486
https://issues.apache.org/jira/browse/HIVE-5486
Repository: hive-git
Description
-------
With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
The patch is creating base scratch dirs at startup if it doesn't exist.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d0895e1
service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
Diff: https://reviews.apache.org/r/14523/diff/
Testing
-------
Added new test
Thanks,
Prasad Mujumdar
Re: Review Request 14523: HIVE-5486 HiveServer2 should create base scratch
directories at startup
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14523/
-----------------------------------------------------------
(Updated Oct. 8, 2013, 1:34 a.m.)
Review request for hive.
Changes
-------
Reformatted patch
Bugs: HIVE-5486
https://issues.apache.org/jira/browse/HIVE-5486
Repository: hive-git
Description
-------
With impersonation enabled, the same base directory is used by all sessions/queries. For a new deployment, this directory gets created on first invocation by the user running that session. This would cause directory permission conflict for other users.
The patch is creating base scratch dirs at startup if it doesn't exist.
Diffs (updated)
-----
service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338
service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java ae7bb6b
service/src/test/org/apache/hive/service/cli/TestEmbeddedThriftBinaryCLIService.java da325da
service/src/test/org/apache/hive/service/cli/TestScratchDir.java PRE-CREATION
Diff: https://reviews.apache.org/r/14523/diff/
Testing
-------
Added new test
Thanks,
Prasad Mujumdar