----------------------------------------------------------- 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 > >
