----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59862/#review177109 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java Line 71 (original), 71 (patched) <https://reviews.apache.org/r/59862/#comment250654> We should also take care of JDO changes and DB schema changes to remove auto-incrementing for this column on the DB. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java Line 87 (original), 87 (patched) <https://reviews.apache.org/r/59862/#comment250657> Just wondering, if we overlooked the issue of not setting +1, how did our tests pass? mvn test -Dtest=TestSentryStore#testConcurrentUpdateChanges sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java Lines 88-90 (original), 88-92 (patched) <https://reviews.apache.org/r/59862/#comment250656> We should retry our DB operations when there is a conflict of multiple threads trying to insert two different records with the changeId? e.q: (Multiple threads doing this) pm.makePersistent(new MSentryPermChange(1, A)); pm.makePersistent(new MSentryPermChange(1, B)); Does the current logic handle the retry? (retry by upating SentryStore.getLastProcessedChangeIDCore(pm, MSentryPermChange.class) + 1) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java Lines 89 (patched) <https://reviews.apache.org/r/59862/#comment250653> Don't we need to make this? ```code pm.makePersistent(new MSentryPermChange(lastChangeID + 1, (PermissionsUpdate) update)); ``` - Vamsee Yarlagadda On June 6, 2017, 9:52 p.m., Lei Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59862/ > ----------------------------------------------------------- > > (Updated June 6, 2017, 9:52 p.m.) > > > Review request for sentry and Alexander Kolbasov. > > > Bugs: SENTRY-1795 > https://issues.apache.org/jira/browse/SENTRY-1795 > > > Repository: sentry > > > Description > ------- > > Previously we change to rely on ORM and database to auto assign primary key, > which will leave holes between keys. SENTRY-1792 explains how we can have > holes in the delta tables. We should plug these holes. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java > d11f37f0157f96b900917d946d05d6b11eeee582 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java > 1cb1a1f00fd7b8c5af49ec5c8bd5c7cdcd5e3a59 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java > 77282da910a16ab5c77575022722988c4fa05073 > > > Diff: https://reviews.apache.org/r/59862/diff/1/ > > > Testing > ------- > > mvn test -Dtest=TestSentryStore#testConcurrentUpdateChanges > > Passed on my laptop. > > > Thanks, > > Lei Xu > >
