> On June 6, 2017, 3:17 p.m., Vamsee Yarlagadda wrote:
> > 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/diff/1/?file=1743371#file1743371line71>
> >
> > We should also take care of JDO changes and DB schema changes to remove
> > auto-incrementing for this column on the DB.
I changed {{package.jdo}} in the new patch. DB schemas are good that they only
set change ID as primary key, but did not set it as auto incr in the schema.
> On June 6, 2017, 3:17 p.m., Vamsee Yarlagadda wrote:
> > 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/diff/1/?file=1743373#file1743373line87>
> >
> > Just wondering, if we overlooked the issue of not setting +1, how did
> > our tests pass?
> >
> > mvn test -Dtest=TestSentryStore#testConcurrentUpdateChanges
Found the reason, it is due to that I haven't remove ```value-strategy``` from
```package.jd```.
And I also change to use ```executeTransactionBlocksWithRetry``` in
```TestSentryStore#testConcurrentUpdateChanges```.
The product code is good that ```SentryStore#execute``` uses
```executeTransactionBlocksWithRetry```.
> On June 6, 2017, 3:17 p.m., Vamsee Yarlagadda wrote:
> > 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/diff/1/?file=1743373#file1743373line88>
> >
> > 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)
Yes, ```SentryStore#execute uses executeTransactionBlocksWithRetry```.
In the production deployment, the concurrency would not be as much as in the
test.
> On June 6, 2017, 3:17 p.m., Vamsee Yarlagadda wrote:
> > 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/diff/1/?file=1743373#file1743373line89>
> >
> > Don't we need to make this?
> > ```code
> > pm.makePersistent(new MSentryPermChange(lastChangeID + 1,
> > (PermissionsUpdate) update));
> > ```
Thanks, fixed.
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59862/#review177109
-----------------------------------------------------------
On June 6, 2017, 2: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, 2: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
>
>
