You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/05/17 19:19:03 UTC
Review Request 59350: SENTRY-1743: Two SentrySTore instances are one
too many
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/
-----------------------------------------------------------
Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Bugs: SENTRY-1743
https://issues.apache.org/jira/browse/SENTRY-1743
Repository: sentry
Description
-------
SENTRY-1743: Two SentrySTore instances are one too many
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
Diff: https://reviews.apache.org/r/59350/diff/1/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On May 17, 2017, 7:37 p.m., Vamsee Yarlagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line145>
> >
> > Shouldn't we call store.stop() to close down all the PersistenceManagerFactory that was initialized as part of constructor?
Good poinnt, should do! And rename it to close() as well. We are not stopping anything.
> On May 17, 2017, 7:37 p.m., Vamsee Yarlagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 149 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line149>
> >
> > If it is not null, call stop() on that resource?
Good point! And we should rename it to close() as well.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review175302
-----------------------------------------------------------
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review175302
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 145 (patched)
<https://reviews.apache.org/r/59350/#comment248756>
Shouldn't we call store.stop() to close down all the PersistenceManagerFactory that was initialized as part of constructor?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 148 (patched)
<https://reviews.apache.org/r/59350/#comment248755>
Start the name in lowercase?
"resetSentryStore"
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 149 (patched)
<https://reviews.apache.org/r/59350/#comment248757>
If it is not null, call stop() on that resource?
- Vamsee Yarlagadda
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentryStore instances are
one too many
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review176861
-----------------------------------------------------------
Ship it!
Ship It!
- Vamsee Yarlagadda
On June 2, 2017, 11:29 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 11:29 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 2ca70e9885145ebee9765a1122eb23d7b1c404fc
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 3327e687f85aa0a5d40fcf299e3967d555a68d17
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java d1a88b0a91a785f68231eaa78e5b1ad8c3071150
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java 103dbb609357d75313abff2228f2f4e15bce1ecf
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentryStore instances are
one too many
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review176918
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On June 2, 2017, 11:29 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 11:29 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 2ca70e9885145ebee9765a1122eb23d7b1c404fc
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 3327e687f85aa0a5d40fcf299e3967d555a68d17
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java d1a88b0a91a785f68231eaa78e5b1ad8c3071150
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java 103dbb609357d75313abff2228f2f4e15bce1ecf
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentryStore instances are
one too many
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/
-----------------------------------------------------------
(Updated June 2, 2017, 11:29 p.m.)
Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Changes
-------
Renamed static constructor to getInstance()
Summary (updated)
-----------------
SENTRY-1743: Two SentryStore instances are one too many
Bugs: SENTRY-1743
https://issues.apache.org/jira/browse/SENTRY-1743
Repository: sentry
Description
-------
SENTRY-1743: Two SentrySTore instances are one too many
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 2ca70e9885145ebee9765a1122eb23d7b1c404fc
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 3327e687f85aa0a5d40fcf299e3967d555a68d17
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java d1a88b0a91a785f68231eaa78e5b1ad8c3071150
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java 103dbb609357d75313abff2228f2f4e15bce1ecf
sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
Diff: https://reviews.apache.org/r/59350/diff/2/
Changes: https://reviews.apache.org/r/59350/diff/1-2/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On May 17, 2017, 8:09 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 86 (original), 88 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line88>
> >
> > SentryService.java has created SentryStore "this.sentryStore = new SentryStore(conf);". Should you update that file as well?
I don't see this code in SentryService. Are you referring to the master branch or sentry-ha-redesign branch?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review175311
-----------------------------------------------------------
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review175311
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 86 (original), 88 (patched)
<https://reviews.apache.org/r/59350/#comment248767>
SentryService.java has created SentryStore "this.sentryStore = new SentryStore(conf);". Should you update that file as well?
- Na Li
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On May 17, 2017, 8:02 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 134 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line134>
> >
> > What is the desirable behavior if the input conf is different from the original conf used to create SentryStore?
This is what happens for tests (but not for prod code) - usually tests want an updated config to be used. That's why we provide the reset mechanism.
> On May 17, 2017, 8:02 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line145>
> >
> > Could you simplify it to be "instance.compareAndSet(null); return instance.get()"? If the store is set, instance.get() returns store. It is easier to read the code.
This wouldn't be correct - the store may be not set in which case instance.get() will return null. This code is updated a bit in the new patch.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review175305
-----------------------------------------------------------
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review175305
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 134 (patched)
<https://reviews.apache.org/r/59350/#comment248761>
What is the desirable behavior if the input conf is different from the original conf used to create SentryStore?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 145 (patched)
<https://reviews.apache.org/r/59350/#comment248758>
Could you simplify it to be "instance.compareAndSet(null); return instance.get()"? If the store is set, instance.get() returns store. It is easier to read the code.
- Na Li
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On May 31, 2017, 7:27 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 134 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line134>
> >
> > The method name does not match with what it is return. Some people may see newSentryStore() as a method that creates a new SentryStore every time is called, but this creates it only once. Should we rename the method? Perhaps have a singleton class?
How about renaming it to getSentryStore()
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review176501
-----------------------------------------------------------
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On May 31, 2017, 7:27 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 134 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line134>
> >
> > The method name does not match with what it is return. Some people may see newSentryStore() as a method that creates a new SentryStore every time is called, but this creates it only once. Should we rename the method? Perhaps have a singleton class?
>
> Alexander Kolbasov wrote:
> How about renaming it to getSentryStore()
>
> Sergio Pena wrote:
> That looks good. What about SentryStore.getInstance() ? I've seen this name on singleton examples.
getInstance() looks good. Will change the name.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review176501
-----------------------------------------------------------
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Sergio Pena <se...@cloudera.com>.
> On May 31, 2017, 7:27 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 134 (patched)
> > <https://reviews.apache.org/r/59350/diff/1/?file=1723426#file1723426line134>
> >
> > The method name does not match with what it is return. Some people may see newSentryStore() as a method that creates a new SentryStore every time is called, but this creates it only once. Should we rename the method? Perhaps have a singleton class?
>
> Alexander Kolbasov wrote:
> How about renaming it to getSentryStore()
That looks good. What about SentryStore.getInstance() ? I've seen this name on singleton examples.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review176501
-----------------------------------------------------------
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 59350: SENTRY-1743: Two SentrySTore instances are
one too many
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59350/#review176501
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 134 (patched)
<https://reviews.apache.org/r/59350/#comment249885>
The method name does not match with what it is return. Some people may see newSentryStore() as a method that creates a new SentryStore every time is called, but this creates it only once. Should we rename the method? Perhaps have a singleton class?
- Sergio Pena
On May 17, 2017, 7:19 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59350/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 7:19 p.m.)
>
>
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1743
> https://issues.apache.org/jira/browse/SENTRY-1743
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1743: Two SentrySTore instances are one too many
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0d2a6b3a74d8d88976aeec32bba901d02e8329f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java 8a8bbd35c1e327fb8b613311daaafef46561b2bb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e9d9eec3526f2894199e81a253709dc3ed380b5b
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 67de5acb36797b23261e5023c308222ad3344ea7
>
>
> Diff: https://reviews.apache.org/r/59350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>