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/05/19 18:56:55 UTC
Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Bugs: SENTRY-1771
https://issues.apache.org/jira/browse/SENTRY-1771
Repository: sentry
Description
-------
when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
Diffs
-----
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
Diff: https://reviews.apache.org/r/59414/diff/1/
Testing
-------
Thanks,
Na Li
Re: Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/#review176505
-----------------------------------------------------------
Can you explain what problem you are actually addressing with this fix?
- Alexander Kolbasov
On May 19, 2017, 6:56 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59414/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:56 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1771
> https://issues.apache.org/jira/browse/SENTRY-1771
>
>
> Repository: sentry
>
>
> Description
> -------
>
> when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
> Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
>
>
> Diff: https://reviews.apache.org/r/59414/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
Posted by Sergio Pena <se...@cloudera.com>.
> On May 19, 2017, 8:22 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
> > Line 183 (original), 185 (patched)
> > <https://reviews.apache.org/r/59414/diff/1/?file=1725397#file1725397line185>
> >
> > I read that the scheduled thread guarantees that this run() is executed sequeantially, and no other tasks will be run at the same time.
> >
> > How is another thread scheduled to run at the same time?
> >
> > Do we have test cases that prooves this?
>
> Na Li wrote:
> We don't have test case prove multiple threads.
Can you write a test to verify that behavior? Even if we don't commit the test, at lest to verify the contract with the scheduled thread.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/#review175567
-----------------------------------------------------------
On May 19, 2017, 6:56 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59414/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:56 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1771
> https://issues.apache.org/jira/browse/SENTRY-1771
>
>
> Repository: sentry
>
>
> Description
> -------
>
> when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
> Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
>
>
> Diff: https://reviews.apache.org/r/59414/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On May 19, 2017, 8:22 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
> > Lines 217 (patched)
> > <https://reviews.apache.org/r/59414/diff/1/?file=1725397#file1725397line217>
> >
> > If the thread is scheduled to be run in 500ms (refreshIntervalMillisec), how is this waitUtil going to work? I think this line is not needed as run() will be run in 500ms and the waitUntil will be already valid at that time.
I suspect that these waits in the run() method are nonsense that should be removed, but they don't explain what we are seeing.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/#review175567
-----------------------------------------------------------
On May 19, 2017, 6:56 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59414/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:56 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1771
> https://issues.apache.org/jira/browse/SENTRY-1771
>
>
> Repository: sentry
>
>
> Description
> -------
>
> when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
> Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
>
>
> Diff: https://reviews.apache.org/r/59414/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
Posted by Na Li <li...@cloudera.com>.
> On May 19, 2017, 8:22 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
> > Lines 217 (patched)
> > <https://reviews.apache.org/r/59414/diff/1/?file=1725397#file1725397line217>
> >
> > If the thread is scheduled to be run in 500ms (refreshIntervalMillisec), how is this waitUtil going to work? I think this line is not needed as run() will be run in 500ms and the waitUntil will be already valid at that time.
>
> Alexander Kolbasov wrote:
> I suspect that these waits in the run() method are nonsense that should be removed, but they don't explain what we are seeing.
the waitUntil is used to wait longer if previous run failed. In normal case, it is not useful
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/#review175567
-----------------------------------------------------------
On May 19, 2017, 6:56 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59414/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:56 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1771
> https://issues.apache.org/jira/browse/SENTRY-1771
>
>
> Repository: sentry
>
>
> Description
> -------
>
> when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
> Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
>
>
> Diff: https://reviews.apache.org/r/59414/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
Posted by Na Li <li...@cloudera.com>.
> On May 19, 2017, 8:22 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
> > Line 183 (original), 185 (patched)
> > <https://reviews.apache.org/r/59414/diff/1/?file=1725397#file1725397line185>
> >
> > I read that the scheduled thread guarantees that this run() is executed sequeantially, and no other tasks will be run at the same time.
> >
> > How is another thread scheduled to run at the same time?
> >
> > Do we have test cases that prooves this?
We don't have test case prove multiple threads.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/#review175567
-----------------------------------------------------------
On May 19, 2017, 6:56 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59414/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:56 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1771
> https://issues.apache.org/jira/browse/SENTRY-1771
>
>
> Repository: sentry
>
>
> Description
> -------
>
> when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
> Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
>
>
> Diff: https://reviews.apache.org/r/59414/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59414: SENTRY-1771: HDFS client requests full
permission update multiple times.
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59414/#review175567
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 183 (original), 185 (patched)
<https://reviews.apache.org/r/59414/#comment248959>
I read that the scheduled thread guarantees that this run() is executed sequeantially, and no other tasks will be run at the same time.
How is another thread scheduled to run at the same time?
Do we have test cases that prooves this?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 217 (patched)
<https://reviews.apache.org/r/59414/#comment248958>
If the thread is scheduled to be run in 500ms (refreshIntervalMillisec), how is this waitUtil going to work? I think this line is not needed as run() will be run in 500ms and the waitUntil will be already valid at that time.
- Sergio Pena
On May 19, 2017, 6:56 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59414/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:56 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1771
> https://issues.apache.org/jira/browse/SENTRY-1771
>
>
> Repository: sentry
>
>
> Description
> -------
>
> when curent time < wait until, return instead of sleeping. This avoid 6 concurrent requests for full snapshot from HDFS.
> Also synchronize the function SentryAuthorizationInfo.update() to make sure requests always contain the latest changeID
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 90ba721
>
>
> Diff: https://reviews.apache.org/r/59414/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>