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/04/27 19:33:16 UTC

Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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

Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Check if credential provider already has the credential before creating a credential


Diffs
-----

  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 31a309b 


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


Testing
-------


Thanks,

Na Li

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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

> On April 28, 2017, 4:41 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 89 (original), 89 (patched)
> > <https://reviews.apache.org/r/58812/diff/1/?file=1702074#file1702074line89>
> >
> >     Please add a comment explaining the problem

It is added.


- Na


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


On April 28, 2017, 7:54 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> -----------------------------------------------------------
> 
> (Updated April 28, 2017, 7:54 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check if credential provider already has the credential before creating a credential
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d12b0da 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 89 (original), 89 (patched)
<https://reviews.apache.org/r/58812/#comment246360>

    Please add a comment explaining the problem


- Alexander Kolbasov


On April 27, 2017, 7:33 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check if credential provider already has the credential before creating a credential
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 31a309b 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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

(Updated April 28, 2017, 7:54 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Check if credential provider already has the credential before creating a credential


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d12b0da 


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

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


Testing
-------


Thanks,

Na Li

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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

> On April 28, 2017, 12:47 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 90 (original), 90 (patched)
> > <https://reviews.apache.org/r/58812/diff/1/?file=1702074#file1702074line90>
> >
> >     Do we need to worry about concurrent access? Can some other thread modify it while we read?

Each thread has its own provider instance, so we don't need to worry about the concurrent access of the provider instance. Those provider instances share the same user, which is static loginUser. That is why when first provider creates a credential, it flushes the credential to the user. Then the second provider gets credential from the user, and if we still create the credential to the second provider, exception is thrown because such credential is present already


- Na


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


On April 27, 2017, 7:33 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check if credential provider already has the credential before creating a credential
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 31a309b 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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

> On April 28, 2017, 12:47 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 90 (original), 90 (patched)
> > <https://reviews.apache.org/r/58812/diff/1/?file=1702074#file1702074line90>
> >
> >     Do we need to worry about concurrent access? Can some other thread modify it while we read?
> 
> Na Li wrote:
>     Each thread has its own provider instance, so we don't need to worry about the concurrent access of the provider instance. Those provider instances share the same user, which is static loginUser. That is why when first provider creates a credential, it flushes the credential to the user. Then the second provider gets credential from the user, and if we still create the credential to the second provider, exception is thrown because such credential is present already

provider gets initial credential at its constructor. After that changing shared user should not affect provider. However, provider will add more credential to the user.


- Na


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


On April 27, 2017, 7:33 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check if credential provider already has the credential before creating a credential
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 31a309b 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 90 (original), 90 (patched)
<https://reviews.apache.org/r/58812/#comment246317>

    Do we need to worry about concurrent access? Can some other thread modify it while we read?


- Alexander Kolbasov


On April 27, 2017, 7:33 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check if credential provider already has the credential before creating a credential
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 31a309b 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>