You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Chaoyu Tang <ct...@gmail.com> on 2016/03/02 17:25:10 UTC
Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-12270
https://issues.apache.org/jira/browse/HIVE-12270
Repository: hive-git
Description
-------
Add HMS APIs to support DB delegation token in HS2.
Only upload the patch without other thrift generated files for review here.
Diffs
-----
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
metastore/if/hive_metastore.thrift e8f0a68
metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java bcc7790
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
Diff: https://reviews.apache.org/r/44271/diff/
Testing
-------
Manuall tests
New Unit test TestJdbcWithDBToken
Precommit tests
Thanks,
Chaoyu Tang
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/#review122019
-----------------------------------------------------------
Yea I studied the code a little bit after our discussion, seems like if we just use raw MSC like I suggest then we wil never close it if DbTokenStore is invoked by thrift threads.. whereas Hive object does have a static close call later in the thread's life.
So let's go with the previous patch, it sucks to have to cache and pass the Hive object as its designed to be thread-local, but it's better than risking HMS leaks.
Sorry for the long discussion with you about it.
- Szehon Ho
On March 4, 2016, 3:24 a.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44271/
> -----------------------------------------------------------
>
> (Updated March 4, 2016, 3:24 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-12270
> https://issues.apache.org/jira/browse/HIVE-12270
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Add HMS APIs to support DB delegation token in HS2.
> Only upload the patch without other thrift generated files for review here.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
> metastore/if/hive_metastore.thrift e8f0a68
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
> metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
>
> Diff: https://reviews.apache.org/r/44271/diff/
>
>
> Testing
> -------
>
> Manuall tests
> New Unit test TestJdbcWithDBToken
> Precommit tests
>
>
> Thanks,
>
> Chaoyu Tang
>
>
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/
-----------------------------------------------------------
(Updated March 5, 2016, 4:20 a.m.)
Review request for hive.
Changes
-------
Revised the patch to getMSC via ThreadLocal Hive object for token api calls in each thread.
Bugs: HIVE-12270
https://issues.apache.org/jira/browse/HIVE-12270
Repository: hive-git
Description
-------
Add HMS APIs to support DB delegation token in HS2.
Only upload the patch without other thrift generated files for review here.
Diffs (updated)
-----
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
itests/hive-unit/src/test/java/org/apache/hadoop/hive/thrift/TestDBTokenStore.java f5934ee
metastore/if/hive_metastore.thrift a4fb612
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 984e3fc
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 3d5e3a4
shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
shims/common/src/main/java/org/apache/hadoop/hive/thrift/HiveDelegationTokenManager.java 9ecb0ee
Diff: https://reviews.apache.org/r/44271/diff/
Testing
-------
Manuall tests
New Unit test TestJdbcWithDBToken
Precommit tests
Thanks,
Chaoyu Tang
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/
-----------------------------------------------------------
(Updated March 4, 2016, 2:25 p.m.)
Review request for hive.
Changes
-------
Upload the original patch with test TestDBTokenStore fixed
Bugs: HIVE-12270
https://issues.apache.org/jira/browse/HIVE-12270
Repository: hive-git
Description
-------
Add HMS APIs to support DB delegation token in HS2.
Only upload the patch without other thrift generated files for review here.
Diffs (updated)
-----
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
itests/hive-unit/src/test/java/org/apache/hadoop/hive/thrift/TestDBTokenStore.java f5934ee
metastore/if/hive_metastore.thrift e8f0a68
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
Diff: https://reviews.apache.org/r/44271/diff/
Testing
-------
Manuall tests
New Unit test TestJdbcWithDBToken
Precommit tests
Thanks,
Chaoyu Tang
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/
-----------------------------------------------------------
(Updated March 4, 2016, 3:24 a.m.)
Review request for hive.
Changes
-------
Revised the patch not to pass around Hive Object but MSC instead based on review feedback. Thanks Szehon
Bugs: HIVE-12270
https://issues.apache.org/jira/browse/HIVE-12270
Repository: hive-git
Description
-------
Add HMS APIs to support DB delegation token in HS2.
Only upload the patch without other thrift generated files for review here.
Diffs (updated)
-----
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
metastore/if/hive_metastore.thrift e8f0a68
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
Diff: https://reviews.apache.org/r/44271/diff/
Testing
-------
Manuall tests
New Unit test TestJdbcWithDBToken
Precommit tests
Thanks,
Chaoyu Tang
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Szehon Ho <sz...@cloudera.com>.
> On March 4, 2016, 12:37 a.m., Szehon Ho wrote:
> > service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java, line 129
> > <https://reviews.apache.org/r/44271/diff/2/?file=1277523#file1277523line129>
> >
> > I'm a bit afraid of concurrency issues by caching Hive. There seems to be quite some issues with Hive object lately and multi-threading and caching seems discouraged now (see HIVE-13002, HIVE-13194, HIVE-13150)
> >
> > Can we have DbTokenStore get Hive on demand via thread-local when it needs it, say if hmsHandler is not passed in?
> >
> > And also can you double-check if it will not leak, ie Hive object is closed somehow by the thread once its done?
Or I guess ServerMode solves that as well.. we can just do Hive.get instead of use the passed-in object if its HS2 mode?
- Szehon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/#review121974
-----------------------------------------------------------
On March 2, 2016, 4:39 p.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44271/
> -----------------------------------------------------------
>
> (Updated March 2, 2016, 4:39 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-12270
> https://issues.apache.org/jira/browse/HIVE-12270
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Add HMS APIs to support DB delegation token in HS2.
> Only upload the patch without other thrift generated files for review here.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
> metastore/if/hive_metastore.thrift e8f0a68
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
> metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
>
> Diff: https://reviews.apache.org/r/44271/diff/
>
>
> Testing
> -------
>
> Manuall tests
> New Unit test TestJdbcWithDBToken
> Precommit tests
>
>
> Thanks,
>
> Chaoyu Tang
>
>
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Chaoyu Tang <ct...@gmail.com>.
> On March 4, 2016, 12:37 a.m., Szehon Ho wrote:
> > service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java, line 129
> > <https://reviews.apache.org/r/44271/diff/2/?file=1277523#file1277523line129>
> >
> > I'm a bit afraid of concurrency issues by caching Hive. There seems to be quite some issues with Hive object lately and multi-threading and caching seems discouraged now (see HIVE-13002, HIVE-13194, HIVE-13150)
> >
> > Can we have DbTokenStore get Hive on demand via thread-local when it needs it, say if hmsHandler is not passed in?
> >
> > And also can you double-check if it will not leak, ie Hive object is closed somehow by the thread once its done?
>
> Szehon Ho wrote:
> Or I guess ServerMode solves that as well.. we can just do Hive.get instead of use the passed-in object if its HS2 mode?
Hi Szehon, Thanks for review.
Yeah, initally I also thought to use Hive.get to get (actually initiate) a threadLocal Hive object and its contained MetaStoreClient in DBTokenStore. But I changed the idea because these token API calls usually happen before a session is opened, and the HMS connection opened for them is usually different from that used in session, since the HMS connection used later in the session may have a different user credential rather than the HS2 owner hive. So this HMS connection can not be reused in the session/queries.
The Hive object now got and used for token APIs is HS2 server main thread local, its Hive.get is called in new HiveAuthFactory(hiveConf) <- ThriftBinaryCLIService.run() <-HiveServer2.start() HS2 thread, it won't be used by any session/queries which run in different threads. The only one other use of this Hive object (or MSC) is CLIService.applyAuthorizationConfigPolicy in CLIService.init during HS2 start, so this Hive object in the main thread is currently only used for token APIs during runtime, and it should not have concrruency issue. In addition, we only use MetaStoreClient in Hive object and not other instance variable values, and the MetaStoreClient is a synchronized client (See Hive getMSC method, HiveMetaStoreClient.newSynchronizedClient(metaStoreClient)), so I think passing in Hive (or HiveMetaStoreClient) and sharing it for token API calls should be quite safe and also save a lot of HMS connections.
I wonder above consideration makes sense or not. Thanks.
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/#review121974
-----------------------------------------------------------
On March 2, 2016, 4:39 p.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44271/
> -----------------------------------------------------------
>
> (Updated March 2, 2016, 4:39 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-12270
> https://issues.apache.org/jira/browse/HIVE-12270
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Add HMS APIs to support DB delegation token in HS2.
> Only upload the patch without other thrift generated files for review here.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
> metastore/if/hive_metastore.thrift e8f0a68
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
> metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
>
> Diff: https://reviews.apache.org/r/44271/diff/
>
>
> Testing
> -------
>
> Manuall tests
> New Unit test TestJdbcWithDBToken
> Precommit tests
>
>
> Thanks,
>
> Chaoyu Tang
>
>
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/#review121974
-----------------------------------------------------------
Conceptually patch looks good, but had the following question below.
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java (line 127)
<https://reviews.apache.org/r/44271/#comment183830>
I'm a bit afraid of concurrency issues by caching Hive. There seems to be quite some issues with Hive object lately and multi-threading and caching seems discouraged now (see HIVE-13002, HIVE-13194, HIVE-13150)
Can we have DbTokenStore get Hive on demand via thread-local when it needs it, say if hmsHandler is not passed in?
And also can you double-check if it will not leak, ie Hive object is closed somehow by the thread once its done?
- Szehon Ho
On March 2, 2016, 4:39 p.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44271/
> -----------------------------------------------------------
>
> (Updated March 2, 2016, 4:39 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-12270
> https://issues.apache.org/jira/browse/HIVE-12270
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Add HMS APIs to support DB delegation token in HS2.
> Only upload the patch without other thrift generated files for review here.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
> metastore/if/hive_metastore.thrift e8f0a68
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
> metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
>
> Diff: https://reviews.apache.org/r/44271/diff/
>
>
> Testing
> -------
>
> Manuall tests
> New Unit test TestJdbcWithDBToken
> Precommit tests
>
>
> Thanks,
>
> Chaoyu Tang
>
>
Re: Review Request 44271: HIVE-12270 : Add DBTokenStore support to HS2
delegation token
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44271/
-----------------------------------------------------------
(Updated March 2, 2016, 4:39 p.m.)
Review request for hive.
Changes
-------
remove the empty spaces from patch
Bugs: HIVE-12270
https://issues.apache.org/jira/browse/HIVE-12270
Repository: hive-git
Description
-------
Add HMS APIs to support DB delegation token in HS2.
Only upload the patch without other thrift generated files for review here.
Diffs (updated)
-----
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java dedbf35
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithDBTokenStore.java PRE-CREATION
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcWithMiniKdc.java 3ef2ce3
metastore/if/hive_metastore.thrift e8f0a68
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bfebfdc
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b5c4d1d
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java cb092d1
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java 0c7455d
shims/common/src/main/java/org/apache/hadoop/hive/thrift/DBTokenStore.java de39d3d
Diff: https://reviews.apache.org/r/44271/diff/
Testing
-------
Manuall tests
New Unit test TestJdbcWithDBToken
Precommit tests
Thanks,
Chaoyu Tang