You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li <li...@cloudera.com> on 2017/06/28 21:03:16 UTC

Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/
-----------------------------------------------------------

Review request for sentry and Alexander Kolbasov.


Repository: sentry


Description
-------

It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.

Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.


Diffs
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 


Diff: https://reviews.apache.org/r/60517/diff/1/


Testing
-------


Thanks,

Na Li


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/#review179308
-----------------------------------------------------------


Ship it!




Ship It!

- Alexander Kolbasov


On June 29, 2017, 8:02 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60517/
> -----------------------------------------------------------
> 
> (Updated June 29, 2017, 8:02 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.
> 
> Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 
> 
> 
> Diff: https://reviews.apache.org/r/60517/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/
-----------------------------------------------------------

(Updated June 29, 2017, 8:02 p.m.)


Review request for sentry and Alexander Kolbasov.


Repository: sentry


Description
-------

It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.

Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 


Diff: https://reviews.apache.org/r/60517/diff/4/

Changes: https://reviews.apache.org/r/60517/diff/3-4/


Testing
-------


Thanks,

Na Li


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/
-----------------------------------------------------------

(Updated June 29, 2017, 5:45 p.m.)


Review request for sentry and Alexander Kolbasov.


Repository: sentry


Description
-------

It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.

Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 


Diff: https://reviews.apache.org/r/60517/diff/3/

Changes: https://reviews.apache.org/r/60517/diff/2-3/


Testing
-------


Thanks,

Na Li


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Na Li <li...@cloudera.com>.

> On June 29, 2017, 4:58 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 103 (patched)
> > <https://reviews.apache.org/r/60517/diff/2/?file=1766946#file1766946line103>
> >
> >     Since you are explaining the issue here, you may just refer to SENTRY-1824 for more details.
> >     
> >     Please use HTML formatting for javadoc.

updated


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/#review179273
-----------------------------------------------------------


On June 29, 2017, 5:45 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60517/
> -----------------------------------------------------------
> 
> (Updated June 29, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.
> 
> Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 
> 
> 
> Diff: https://reviews.apache.org/r/60517/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/#review179273
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 103 (patched)
<https://reviews.apache.org/r/60517/#comment253924>

    Since you are explaining the issue here, you may just refer to SENTRY-1824 for more details.
    
    Please use HTML formatting for javadoc.


- Alexander Kolbasov


On June 29, 2017, 3:44 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60517/
> -----------------------------------------------------------
> 
> (Updated June 29, 2017, 3:44 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.
> 
> Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 
> 
> 
> Diff: https://reviews.apache.org/r/60517/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/
-----------------------------------------------------------

(Updated June 29, 2017, 3:44 p.m.)


Review request for sentry and Alexander Kolbasov.


Repository: sentry


Description
-------

It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.

Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 


Diff: https://reviews.apache.org/r/60517/diff/2/

Changes: https://reviews.apache.org/r/60517/diff/1-2/


Testing
-------


Thanks,

Na Li


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Na Li <li...@cloudera.com>.

> On June 29, 2017, 12:43 a.m., Alexander Kolbasov wrote:
> > Please add a comment explaining why these calls are synchronized.

Explaination is added at class comment


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/#review179211
-----------------------------------------------------------


On June 29, 2017, 3:44 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60517/
> -----------------------------------------------------------
> 
> (Updated June 29, 2017, 3:44 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.
> 
> Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 
> 
> 
> Diff: https://reviews.apache.org/r/60517/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>


Re: Review Request 60517: SENTRY-1824: SentryStore may serialize transactions that rely on unique key

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60517/#review179211
-----------------------------------------------------------



Please add a comment explaining why these calls are synchronized.

- Alexander Kolbasov


On June 28, 2017, 9:03 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60517/
> -----------------------------------------------------------
> 
> (Updated June 28, 2017, 9:03 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> It would be the best if we can minimize transaction failure when two or more transactions are making updates. Permission updates are the main issue, getting worse with more clients. Path update is from one thread in HMSFollower, independent on the number of clients, so it is not a big issue.
> 
> Add synchronization for permission updates? In this way, when more than one threads are updating the permission, only one thread will update changeID, when it finishes, the next thread on the same Sentry server will make update. Therefore, no transaction will fail if there is only one Sentry server. When there are more Sentry servers, the collision will happen among the Sentry servers, not depending on how many clients are updating Sentry, so it is more scalable.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 9ad97bc 
> 
> 
> Diff: https://reviews.apache.org/r/60517/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>