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 2014/12/09 03:09:59 UTC
Re: Review Request 25981: SENTRY-457 Global Sequence ID support for
SENTRY high availability
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25981/#review64322
-----------------------------------------------------------
The patch itself looks fine.
I do have a bit of concern about mixing the ZK operation with DB transaction. This essentially makes the Sentry RPC a distributed transaction, between SentryStore and ZK. Since there no 2 phase commit, it's always possible to end up with local sequence number (the patch uses the local seq id if the ZK counter can't be obtained).
What are your thoughts on getting the sequence from the database itself, eg using sequence object of a simple countered stored in the DB ?
- Prasad Mujumdar
On Oct. 27, 2014, 6:54 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25981/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2014, 6:54 a.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-457
> https://issues.apache.org/jira/browse/SENTRY-457
>
>
> Repository: sentry
>
>
> Description
> -------
>
> As **CommitContext** contains a sequenceId, it will auto-increment per operation, In HA environment, we should make it work, also add a distributed counter
> * Add **CommitContextFactory** , use **getCommitContext(UUID serverUUID, long sequenceId)** in the factory instead of **new CommitContext(UUID serverUUID, long sequenceId)**
> * **counter.getGlobalSequenceId()** will return the Global sequence id
> ````java
> public CommitContext getCommitContext(UUID serverUUID, long sequenceId) {
> if (haEnabled) {
> try {
> return new CommitContext(serverUUID, counter.getGlobalSequenceId());
> } catch (Exception e) {
> LOGGER.error("Error in get global sequence Id, return the sequenceId in service!" ,e);
> return new CommitContext(serverUUID, sequenceId);
> }
> } else {
> return new CommitContext(serverUUID, sequenceId);
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CommitContextFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 017cf08
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 52eaeed
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryDistributeCounter.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25981/diff/
>
>
> Testing
> -------
>
> Unit Test passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25981: SENTRY-457 Global Sequence ID support for
SENTRY high availability
Posted by Dapeng Sun <da...@intel.com>.
> On 十二月 9, 2014, 10:09 a.m., Prasad Mujumdar wrote:
> > The patch itself looks fine.
> >
> > I do have a bit of concern about mixing the ZK operation with DB transaction. This essentially makes the Sentry RPC a distributed transaction, between SentryStore and ZK. Since there no 2 phase commit, it's always possible to end up with local sequence number (the patch uses the local seq id if the ZK counter can't be obtained).
> > What are your thoughts on getting the sequence from the database itself, eg using sequence object of a simple countered stored in the DB ?
Yes, I'm agreed with you, mixing the ZK operation with DB transaction may cause the problem of synchronization.
And it's a good choice to store the counter in the DB. I will look into it and refactor the code.
Thanks a lot for helping to point out it and give me your good suggestion.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25981/#review64322
-----------------------------------------------------------
On 十月 27, 2014, 2:54 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25981/
> -----------------------------------------------------------
>
> (Updated 十月 27, 2014, 2:54 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-457
> https://issues.apache.org/jira/browse/SENTRY-457
>
>
> Repository: sentry
>
>
> Description
> -------
>
> As **CommitContext** contains a sequenceId, it will auto-increment per operation, In HA environment, we should make it work, also add a distributed counter
> * Add **CommitContextFactory** , use **getCommitContext(UUID serverUUID, long sequenceId)** in the factory instead of **new CommitContext(UUID serverUUID, long sequenceId)**
> * **counter.getGlobalSequenceId()** will return the Global sequence id
> ````java
> public CommitContext getCommitContext(UUID serverUUID, long sequenceId) {
> if (haEnabled) {
> try {
> return new CommitContext(serverUUID, counter.getGlobalSequenceId());
> } catch (Exception e) {
> LOGGER.error("Error in get global sequence Id, return the sequenceId in service!" ,e);
> return new CommitContext(serverUUID, sequenceId);
> }
> } else {
> return new CommitContext(serverUUID, sequenceId);
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CommitContextFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 017cf08
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 52eaeed
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryDistributeCounter.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25981/diff/
>
>
> Testing
> -------
>
> Unit Test passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>