You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2015/02/23 22:11:42 UTC
Review Request 31321: SENTRY-658: Connection leak in Hive biding with
Sentry service
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31321/
-----------------------------------------------------------
Review request for sentry, Arun Suresh and Lenni Kuff.
Bugs: SENTRY-658
https://issues.apache.org/jira/browse/SENTRY-658
Repository: sentry
Description
-------
Ensure that the hive binding client is closed at the end of the operation
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java 0cbb250
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e311398
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java b4b69e1
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 5e26e83
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 0b4b39a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 68bc9ee
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java e9ae5fa
Diff: https://reviews.apache.org/r/31321/diff/
Testing
-------
- Updated miniSentry framework to track active clients
- Added new test to verify client disconnection for various Hive operations
Thanks,
Prasad Mujumdar
Re: Review Request 31321: SENTRY-658: Connection leak in Hive biding
with Sentry service
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31321/
-----------------------------------------------------------
(Updated Feb. 24, 2015, 8:54 a.m.)
Review request for sentry, Arun Suresh and Lenni Kuff.
Changes
-------
Changes per review feedback
Bugs: SENTRY-658
https://issues.apache.org/jira/browse/SENTRY-658
Repository: sentry
Description
-------
Ensure that the hive binding client is closed at the end of the operation
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java 0cbb250
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e311398
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java b4b69e1
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 5e26e83
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 0b4b39a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 68bc9ee
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java e9ae5fa
Diff: https://reviews.apache.org/r/31321/diff/
Testing
-------
- Updated miniSentry framework to track active clients
- Added new test to verify client disconnection for various Hive operations
Thanks,
Prasad Mujumdar
Re: Review Request 31321: SENTRY-658: Connection leak in Hive biding
with Sentry service
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On Feb. 24, 2015, 8:01 a.m., Lenni Kuff wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java, line 68
> > <https://reviews.apache.org/r/31321/diff/1/?file=873116#file873116line68>
> >
> > is it possible to verify that this gets incremented to 1 when there is an active connection?
Not directly since that happens during the execution of statement. However track the client ids so it shouldn't be hard to verify that the client actually got connected by checking the ids.
will add the check.
> On Feb. 24, 2015, 8:01 a.m., Lenni Kuff wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java, line 60
> > <https://reviews.apache.org/r/31321/diff/1/?file=873117#file873117line60>
> >
> > Does this need to be thread safe (I think it does). If so, I think you need to protect clientList. Should clientList actually be a Set though?
> >
> > If this does not need to be thread safe, change these from AtomicLong long for claririty.
> >
> > Comment on thread safety.
For the current test it doesn't need to be thread safe, but in general it should be. will add a comment.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31321/#review73769
-----------------------------------------------------------
On Feb. 23, 2015, 9:11 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31321/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 9:11 p.m.)
>
>
> Review request for sentry, Arun Suresh and Lenni Kuff.
>
>
> Bugs: SENTRY-658
> https://issues.apache.org/jira/browse/SENTRY-658
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Ensure that the hive binding client is closed at the end of the operation
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java 0cbb250
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e311398
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java b4b69e1
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 5e26e83
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 0b4b39a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 68bc9ee
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java e9ae5fa
>
> Diff: https://reviews.apache.org/r/31321/diff/
>
>
> Testing
> -------
>
> - Updated miniSentry framework to track active clients
> - Added new test to verify client disconnection for various Hive operations
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 31321: SENTRY-658: Connection leak in Hive biding
with Sentry service
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31321/#review73769
-----------------------------------------------------------
Ship it!
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java
<https://reviews.apache.org/r/31321/#comment120153>
is it possible to verify that this gets incremented to 1 when there is an active connection?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
<https://reviews.apache.org/r/31321/#comment120154>
Does this need to be thread safe (I think it does). If so, I think you need to protect clientList. Should clientList actually be a Set though?
If this does not need to be thread safe, change these from AtomicLong long for claririty.
Comment on thread safety.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
<https://reviews.apache.org/r/31321/#comment120155>
can you get rid of clientCount and just return clientList.size()?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
<https://reviews.apache.org/r/31321/#comment120156>
Get the
- Lenni Kuff
On Feb. 23, 2015, 9:11 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31321/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 9:11 p.m.)
>
>
> Review request for sentry, Arun Suresh and Lenni Kuff.
>
>
> Bugs: SENTRY-658
> https://issues.apache.org/jira/browse/SENTRY-658
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Ensure that the hive binding client is closed at the end of the operation
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java 0cbb250
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e311398
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java b4b69e1
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 5e26e83
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 0b4b39a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 68bc9ee
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java e9ae5fa
>
> Diff: https://reviews.apache.org/r/31321/diff/
>
>
> Testing
> -------
>
> - Updated miniSentry framework to track active clients
> - Added new test to verify client disconnection for various Hive operations
>
>
> Thanks,
>
> Prasad Mujumdar
>
>