You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lei Xu <le...@cloudera.com> on 2017/04/08 00:38:04 UTC
Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/
-----------------------------------------------------------
Review request for sentry.
Bugs: SENTRY-1643
https://issues.apache.org/jira/browse/SENTRY-1643
Repository: sentry
Description
-------
When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
This patch changes to use application logic to force the consectivity of IDs.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
Diff: https://reviews.apache.org/r/58281/diff/1/
Testing
-------
Add a new test to conurrently insert changes.
mvn test -Dtest=TestSentryStore.
Thanks,
Lei Xu
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
> On April 10, 2017, 9:25 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
> > Lines 87 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686601#file1686601line87>
> >
> > Is it possible for the following scenario to happen?
> >
> > There are two Sentry server instances running. Request A goes to Sentry A, and add a perm update. At the same time HMSFollower at Sentry B gets a path update.
> >
> > Both Sentry A and Sentry B calls this function to persist update. And both get lastChangeID to be, for example, 3. If Sentry A calls "pm.makePersistent" first, the "SentryStore.getLastProcessedChangeIDCore" is 4 now. Then Sentry B calls "pm.makePersistent", and the "SentryStore.getLastProcessedChangeIDCore" remains as 4, but it should be 5 as a new update is persisted (will this cause exception as two update entries are having the same changeID?).
`jdo.makePersistent` is doing `INSERT`. And since `changeID` is the primary ID, only one of the insertation can success. The other one will go the retry in the transaction and pick up the next change ID to persist.
> On April 10, 2017, 9:25 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2581 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686603#file1686603line2581>
> >
> > did you reproduce the original issue using this test code? If not, then it cannot verify the fix for the original issue.
Yes, it did.
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171444
-----------------------------------------------------------
On April 7, 2017, 5:38 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 5:38 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/1/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171444
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
Lines 87 (patched)
<https://reviews.apache.org/r/58281/#comment244381>
Is it possible for the following scenario to happen?
There are two Sentry server instances running. Request A goes to Sentry A, and add a perm update. At the same time HMSFollower at Sentry B gets a path update.
Both Sentry A and Sentry B calls this function to persist update. And both get lastChangeID to be, for example, 3. If Sentry A calls "pm.makePersistent" first, the "SentryStore.getLastProcessedChangeIDCore" is 4 now. Then Sentry B calls "pm.makePersistent", and the "SentryStore.getLastProcessedChangeIDCore" remains as 4, but it should be 5 as a new update is persisted (will this cause exception as two update entries are having the same changeID?).
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2581 (patched)
<https://reviews.apache.org/r/58281/#comment244382>
did you reproduce the original issue using this test code? If not, then it cannot verify the fix for the original issue.
- Na Li
On April 8, 2017, 12:38 a.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 8, 2017, 12:38 a.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/1/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171454
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On April 8, 2017, 12:38 a.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 8, 2017, 12:38 a.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/1/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/
-----------------------------------------------------------
(Updated April 10, 2017, 3:47 p.m.)
Review request for sentry.
Bugs: SENTRY-1643
https://issues.apache.org/jira/browse/SENTRY-1643
Repository: sentry
Description
-------
When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
This patch changes to use application logic to force the consectivity of IDs.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
Diff: https://reviews.apache.org/r/58281/diff/3/
Changes: https://reviews.apache.org/r/58281/diff/2-3/
Testing
-------
Add a new test to conurrently insert changes.
mvn test -Dtest=TestSentryStore.
Thanks,
Lei Xu
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
> On April 10, 2017, 2:57 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2592 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686603#file1686603line2592>
> >
> > I'd suggest to use barrier to make sure all threads start racing at the same time. This will improve chance of getting conflicts.
Done
> On April 10, 2017, 2:57 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2616 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686603#file1686603line2616>
> >
> > it may be better to log it rather then print to stdout
It is removed in the version 2 patch.
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171494
-----------------------------------------------------------
On April 10, 2017, 12:54 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 10, 2017, 12:54 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/2/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171494
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2592 (patched)
<https://reviews.apache.org/r/58281/#comment244423>
I'd suggest to use barrier to make sure all threads start racing at the same time. This will improve chance of getting conflicts.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2606 (patched)
<https://reviews.apache.org/r/58281/#comment244424>
shutdown will cancel all pending jobs - do you need to wait until all are done? Otherwise you might miss some updates
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2616 (patched)
<https://reviews.apache.org/r/58281/#comment244425>
it may be better to log it rather then print to stdout
- Alexander Kolbasov
On April 10, 2017, 7:54 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 10, 2017, 7:54 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/2/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
> On April 10, 2017, 12:58 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
> > Lines 87 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686601#file1686601line87>
> >
> > I agree, functionally this approach will solve ths issue. but this approach will create performance issues under traffic.
> >
> > My statement makes more sence for PERM updates.
> >
> > I have not looked at the logic in namenode plug-in. If we can change that logic to handle monotonically increasing change ID's, that is best way to go.
+1. I agree that this is not the most performant solution, as I hesitated to do so in the first place. Do you know that whether it is applicable to build trigger SQL for each table for every supported DB, that only sets the change ID when the record is inserted successfully?
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171464
-----------------------------------------------------------
On April 10, 2017, 12:54 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 10, 2017, 12:54 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/2/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
> On April 10, 2017, 12:58 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686599#file1686599line75>
> >
> > Please add a comment on how change id is identified.
Done.
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171464
-----------------------------------------------------------
On April 10, 2017, 3:47 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 10, 2017, 3:47 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/3/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171464
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
Lines 75 (patched)
<https://reviews.apache.org/r/58281/#comment244402>
Please add a comment on how change id is identified.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
Lines 87 (patched)
<https://reviews.apache.org/r/58281/#comment244406>
I agree, functionally this approach will solve ths issue. but this approach will create performance issues under traffic.
My statement makes more sence for PERM updates.
I have not looked at the logic in namenode plug-in. If we can change that logic to handle monotonically increasing change ID's, that is best way to go.
- kalyan kumar kalvagadda
On April 10, 2017, 7:54 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 10, 2017, 7:54 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/2/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/
-----------------------------------------------------------
(Updated April 10, 2017, 12:54 p.m.)
Review request for sentry.
Bugs: SENTRY-1643
https://issues.apache.org/jira/browse/SENTRY-1643
Repository: sentry
Description
-------
When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
This patch changes to use application logic to force the consectivity of IDs.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
Diff: https://reviews.apache.org/r/58281/diff/2/
Changes: https://reviews.apache.org/r/58281/diff/1-2/
Testing
-------
Add a new test to conurrently insert changes.
mvn test -Dtest=TestSentryStore.
Thanks,
Lei Xu
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Lei Xu <le...@cloudera.com>.
> On April 10, 2017, 11:37 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2581 (patched)
> > <https://reviews.apache.org/r/58281/diff/1/?file=1686603#file1686603line2581>
> >
> > Can you add a comment for what this test is about? Also a link to the sentry jira number. Thanks!
Addressed in the updated patch.
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171458
-----------------------------------------------------------
On April 7, 2017, 5:38 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 5:38 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/1/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of
MSentryPermChange/MSentryPathChange may be error-prone
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171458
-----------------------------------------------------------
Fix it, then Ship it!
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2581 (patched)
<https://reviews.apache.org/r/58281/#comment244398>
Can you add a comment for what this test is about? Also a link to the sentry jira number. Thanks!
- Hao Hao
On April 8, 2017, 12:38 a.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> -----------------------------------------------------------
>
> (Updated April 8, 2017, 12:38 a.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When it relies on the SQL auto increment primary key as ChangeID, it can not guarentee the consectivity of the IDs, because while each transaction claims new ID from the table counter, the concurrent transactions which were not success would not return the claimed changeIDs to the pool, thus it can not guarateen the consectivity of change IDs.
>
> This patch changes to use application logic to force the consectivity of IDs.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java a0d34459d7b2f70e863ef6e078401df81381c91b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 476fbcb2ad26de23757842111beb12b154e1562b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java f590a5296c047e1acedd39a4f2e4f1de98008d32
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77
>
>
> Diff: https://reviews.apache.org/r/58281/diff/1/
>
>
> Testing
> -------
>
> Add a new test to conurrently insert changes.
>
> mvn test -Dtest=TestSentryStore.
>
>
> Thanks,
>
> Lei Xu
>
>