You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org> on 2018/01/22 13:58:22 UTC

Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

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

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Bugs: SENTRY-1904
    https://issues.apache.org/jira/browse/SENTRY-1904


Repository: sentry


Description
-------

The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 

New configuration parameter that limits the max time spent by transaction retry is added. When the next retry interval exceeds the configured max time, transaction should not retry and return a failure.


Diffs
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 


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


Testing
-------

Made sure all the tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Na Li via Review Board <no...@reviews.apache.org>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Lines 423 (patched)
<https://reviews.apache.org/r/65268/#comment275273>

    Can we combine log message in line 419 with line 423 for duplicate notification and else?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 246 (patched)
<https://reviews.apache.org/r/65268/#comment275292>

    Jira description mentioned "We should also have a constraint on the max time for a transaction so that we do not retry for too long"
    
    This approach is only effective when the current retry interval exceeds the maxRetryWaitTimeMills. It does not directly control the max time a transaction takes. Is it good enough?


- Na Li


On Jan. 22, 2018, 1:58 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 22, 2018, 1:58 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New configuration parameter that limits the max time spent by transaction retry is added. When the next retry interval exceeds the configured max time, transaction should not retry and return a failure.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/1/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Steve Moist via Review Board <no...@reviews.apache.org>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 259 (patched)
<https://reviews.apache.org/r/65268/#comment276116>

    If you're braking out of this anyway, why not just throw an Exception here instead of the break?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 102 (patched)
<https://reviews.apache.org/r/65268/#comment276117>

    Change to 30_000 for readability or 30*1000



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 108 (patched)
<https://reviews.apache.org/r/65268/#comment276118>

    change to 300_000 or 5*60*1000


- Steve Moist


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On Jan. 24, 2018, 7:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 4238 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944702#file1944702line4238>
> >
> >     How is this change related to your changes? This seems to be coming from some other patch.

This method is added as part of this patch only. It is used to log duplicate.


> On Jan. 24, 2018, 7:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 231 (original), 246 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line246>
> >
> >     Why do we need transactionRetryMax if we are limited retries by total time?

transactionRetryMax is used to draw a hard line. If someone configures bigger retry count, this would draw a hard line.
I have added this based on the comments received.


> On Jan. 24, 2018, 7:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 247 (original), 268 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line268>
> >
> >     I think this part needs some adjustment. Since it is exponential it may try to sleep for too long. I think if the value is becoming big we should reset it to a smallish value.

That is what this patch does, if the sleep time between retries goes beyond max retry interval it limits the sleep time to max retry interval. This max retry interval is configurable.


> On Jan. 24, 2018, 7:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2594 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944705#file1944705line2594>
> >
> >     What exactly is this test testing? Given what you are changing this seems to be a weird test for that.

This test similates transaction failures and makes sure that approparite exceptions are raised to verify the changes added.


- kalyan kumar


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


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




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

    How is this change related to your changes? This seems to be coming from some other patch.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 246 (patched)
<https://reviews.apache.org/r/65268/#comment275669>

    Why do we need transactionRetryMax if we are limited retries by total time?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 256 (patched)
<https://reviews.apache.org/r/65268/#comment275671>

    I think it is better to read timestamp at the beginning and compare against current timestamp to check for elapsed times.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 247 (original), 268 (patched)
<https://reviews.apache.org/r/65268/#comment275672>

    I think this part needs some adjustment. Since it is exponential it may try to sleep for too long. I think if the value is becoming big we should reset it to a smallish value.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2594 (patched)
<https://reviews.apache.org/r/65268/#comment275668>

    What exactly is this test testing? Given what you are changing this seems to be a weird test for that.


- Alexander Kolbasov


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.

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


Ship it!




Ship It!

- Vadim Spector


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 110 (original), 115 (patched)
<https://reviews.apache.org/r/65268/#comment276437>

    It would be nice to add comment explaining what's going on with the implementation.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 244 (original), 248 (patched)
<https://reviews.apache.org/r/65268/#comment276438>

    This comment seems misplaced now.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 249 (patched)
<https://reviews.apache.org/r/65268/#comment276439>

    What are you trying to do here? This is a non-trivial expression that requires explanation. Better yet to express it in a way that is obvious.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 250 (patched)
<https://reviews.apache.org/r/65268/#comment276440>

    Why do you want to break here?
    
    I guess what you are trying to do is 
    
        if (startTimeStamp + maxTimeAllowed > currentTime) break;
    
    Or you are trying to do something else?
    
    If it is what you are trying to do it would be nice to pre-compute end time by adding startTimestamp and the limit and just compare curtrent time with the deadline time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 247 (original), 254 (patched)
<https://reviews.apache.org/r/65268/#comment276441>

    It may not be very important but I think incrementing retryCount before sleep is better.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 250 (original), 257 (patched)
<https://reviews.apache.org/r/65268/#comment276442>

    WHy do you need both sleepTime and newSleepTime?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 251 (original), 265 (patched)
<https://reviews.apache.org/r/65268/#comment276443>

    And what if it isn't?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 269 (patched)
<https://reviews.apache.org/r/65268/#comment276444>

    At this point you know how much time you spent, so it would be nice to print it.



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

    This comment is no longer correct



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3421 (patched)
<https://reviews.apache.org/r/65268/#comment276414>

    I think it is better to have separate tests for duplicate notifications and for transactiuon timeout.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3426 (original), 3433 (patched)
<https://reviews.apache.org/r/65268/#comment276421>

    Extra spaces



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3449 (original), 3456 (patched)
<https://reviews.apache.org/r/65268/#comment276418>

    Why do you have extra spaces here?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3451 (original), 3458 (patched)
<https://reviews.apache.org/r/65268/#comment276419>

    Extra spaces



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3453 (original), 3460 (patched)
<https://reviews.apache.org/r/65268/#comment276420>

    Extra spaces



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3481 (original), 3489 (patched)
<https://reviews.apache.org/r/65268/#comment276417>

    WHy do you have spacing changes?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3495 (patched)
<https://reviews.apache.org/r/65268/#comment276415>

    Even with the check it is possible that the time exceeds the allotted time - you need to give some margin of error or this will be a flaky test.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3496 (patched)
<https://reviews.apache.org/r/65268/#comment276416>

    Please provide more information - how much longer did it take? Was it one nanosecond or 2 hours?


- Alexander Kolbasov


On Jan. 31, 2018, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 31, 2018, 12:30 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/5/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 138 (original), 138 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line138>
> >
> >     1) Please add space after //
> >     2) The comment should probably say something like: `These tests do not need to retry transactions, so limit transaction retries to half a second.

will update


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3424 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3424>
> >
> >     Can we use something simpler to test this? What would be the simplest thing that we can use to cause exceptions?

I thought simpler way to simulate this failure was by inserting duplicate entries. That is what I have done. Let me know of you can think anything simpler


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3435 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3435>
> >
> >     This can be just `new HashMap<>()`

will change.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3447 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3447>
> >
> >     It would be better to simplify the message - e.g. 
> >     `fail("expected JDODataStoreException")`

will change.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3449 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3449>
> >
> >     Do you know that it is JDODataStoreException? WIll it be the same if the test is executed on non-Derby DB?

Performing a duplicate insert would result in JDODataStoreException(derived from javax.jdo.JDOCanRetryException) from datanucleus.

This exception is not specific to derby and should be seen even for other databases. Sentry store tries to catch the same exception in many API's.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3450 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3450>
> >
> >     Unexpected failure isn't very useful - it is better to describe what went wrong - what you expected and what happened instead.

Will update.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3453 (patched)
> > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3453>
> >
> >     millisecond is one word

Will update.


- kalyan kumar


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


On Feb. 5, 2018, 3:02 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2018, 3:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/6/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




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

    1) Please add space after //
    2) The comment should probably say something like: `These tests do not need to retry transactions, so limit transaction retries to half a second.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3424 (patched)
<https://reviews.apache.org/r/65268/#comment276700>

    Can we use something simpler to test this? What would be the simplest thing that we can use to cause exceptions?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3435 (patched)
<https://reviews.apache.org/r/65268/#comment276701>

    This can be just `new HashMap<>()`



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3447 (patched)
<https://reviews.apache.org/r/65268/#comment276704>

    It would be better to simplify the message - e.g. 
    `fail("expected JDODataStoreException")`



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3449 (patched)
<https://reviews.apache.org/r/65268/#comment276708>

    Do you know that it is JDODataStoreException? WIll it be the same if the test is executed on non-Derby DB?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3450 (patched)
<https://reviews.apache.org/r/65268/#comment276707>

    Unexpected failure isn't very useful - it is better to describe what went wrong - what you expected and what happened instead.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3453 (patched)
<https://reviews.apache.org/r/65268/#comment276709>

    millisecond is one word


- Alexander Kolbasov


On Feb. 5, 2018, 3:02 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2018, 3:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/6/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 221 (original), 227 (patched)
<https://reviews.apache.org/r/65268/#comment277273>

    Try execurting transaction. If it fails due to SentryUserException, propagate the exception immediately. Otherwise retry transaction multiple times trying not to exceed the total time limit.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 224 (original), 240 (patched)
<https://reviews.apache.org/r/65268/#comment277274>

    I sould suggest something along the following lines:
    
    `
      private class ExponentialBackoff {
    
        @SuppressWarnings("squid:S00112")
        <T> T execute(Callable<T> arg) throws Exception {
          Exception ex = null;
          int retryNum = 0;
          // Maximumm total sleep time
          long sleepTime = maxRetryDurationMills / 32;
          // Maximum sleep time between retries
          final long maxSleep = maxRetryDurationMills / 4;
    
          final long deadline = System.currentTimeMillis() + maxRetryDurationMills;
    
          do {
            try {
              return arg.call();
            } catch (SentryUserException e) {
              // throw the sentry exception without retry
              LOGGER.warn("Transaction manager encountered non-retriable exception", e);
              throw e;
            } catch (Exception e) {
              ex = e;
              LOGGER.warn("Transaction execution encountered exception", e);
              long remainingTime = deadline - System.currentTimeMillis();
              if (remainingTime <= 0) {
                break;
              }
    
              if (sleepTime < maxSleep) {
                // Introduce some randomness in the backoff time.
                int fuzz = random.nextInt((int) sleepTime / 2);
                sleepTime *= 3;
                sleepTime /= 2;
                sleepTime += fuzz;
              }
              sleepTime = Math.min(sleepTime, Math.min(maxSleep, remainingTime));
              LOGGER.warn("Retrying transaction {} times, sleeping for {} ms", ++retryNum, sleepTime);
              retryCount.inc();
              Thread.sleep(sleepTime);
            }
          } while (true);
          assert (ex != null);
          String message = "Retried for " + maxRetryDurationMills + " milliseconds. Giving up "
                  + ex.getMessage();
          LOGGER.error(message, ex);
          throw new Exception(message, ex);
        }
      }
    `


- Alexander Kolbasov


On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2018, 6:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/7/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

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



I left a few comments, but I need to go early so I would leave some comments later.


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 240 (original), 258 (patched)
<https://reviews.apache.org/r/65268/#comment277261>

    Is there a way to get rid of the isLastRetry? Can we avoid it?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 243 (original), 264 (patched)
<https://reviews.apache.org/r/65268/#comment277254>

    Should we store this time in a variable instead of calling 3 times in this if() statement?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 247 (original), 273 (patched)
<https://reviews.apache.org/r/65268/#comment277258>

    Why is this warn() moved after the sleep instead of the original one which was before the sleep? is there a benefit?


- Sergio Pena


On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2018, 6:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/7/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Xinran Tinney <yu...@gmail.com>.


> On Feb. 9, 2018, 9:17 p.m., Xinran Tinney wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3423 (patched)
> > <https://reviews.apache.org/r/65268/diff/7/?file=1954006#file1954006line3423>
> >
> >     "long than"

"longer than" instead of "longer that"


- Xinran


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


On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2018, 6:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/7/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Xinran Tinney <yu...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3423 (patched)
<https://reviews.apache.org/r/65268/#comment277301>

    "long than"



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3461 (patched)
<https://reviews.apache.org/r/65268/#comment277316>

    should be a new line from "if"


- Xinran Tinney


On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2018, 6:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/7/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 262 (patched)
<https://reviews.apache.org/r/65268/#comment278182>

    If the maxRetryTimeIntervalMillis is lower than 1/3 of the maxRetryTime, then we will start seeing the same sleepTime is used in >50% of all retries until the end of the maxDurationTime.
    
    Should we need introduce the randomness after the final sleepTime calculation?
    
    Attempt#1   420
    Attempt#2   704
    Attempt#3  1281
    Attempt#4  2069
    Attempt#5  3498
    Attempt#6  6360
    Attempt#7  7500
    Attempt#8  7500
    Attempt#9  7500
    Attempt#10 7500
    Attempt#11 7500
    Attempt#12 7500
    Attempt#13 7500
    Attempt#14 7500
    Attempt#15 7500
    Attempt#16 7369



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 265 (patched)
<https://reviews.apache.org/r/65268/#comment278177>

    By moving this sleep to the end, the initial sleep time which used to be 250ms is not used but the next time.
    
    The old code used to sleep initially for 250ms.
    The new code is incrementing the sleep time that could be up to 500ms for the initial sleep.
    
    Is this the new expected behavior? Shouldn't we move the sleep() call before making the new sleepTime calculation?


- Sergio Pena


On Feb. 12, 2018, 11:50 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2018, 11:50 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/8/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 106 (patched)
<https://reviews.apache.org/r/65268/#comment278191>

    I would consider changing default here from 250ms to 125 ms. With the current sleep the first delay is closer to 400-500 ms which is a bit too much for initial sleep
    with 125 ms default we get initial sleep close to 200 ms while not increasing retries a lot.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3432 (patched)
<https://reviews.apache.org/r/65268/#comment278192>

    30 sec is too much for this test - it delays the whole test for 30 sec. Can you test it with the whole test taking ~2 seconds?


- Alexander Kolbasov


On Feb. 12, 2018, 11:50 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2018, 11:50 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/8/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

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

(Updated Feb. 12, 2018, 11:50 p.m.)

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.

Changes
-------

Made code changes to address comments from sasha and xinran.

Bugs: SENTRY-1904
https://issues.apache.org/jira/browse/SENTRY-1904

Repository: sentry

Description
-------

The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long.

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.
3.Removed retry based on count.

With out these limits we would not have a control on how long a transaction could be be active.

Diffs (updated)
-----

sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9

Diff: https://reviews.apache.org/r/65268/diff/8/

Changes: https://reviews.apache.org/r/65268/diff/7-8/

Testing
-------

Made sure all the tests pass.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated Feb. 7, 2018, 6:04 p.m.)

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.

Changes
-------

Addressed comments from sasha.

Bugs: SENTRY-1904
https://issues.apache.org/jira/browse/SENTRY-1904

Repository: sentry

Description
-------

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.
3.Removed retry based on count.

With out these limits we would not have a control on how long a transaction could be be active.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/65268/diff/7/

Changes: https://reviews.apache.org/r/65268/diff/6-7/

Testing
-------

Made sure all the tests pass.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 224 (original), 224 (patched)
<https://reviews.apache.org/r/65268/#comment276686>

    1) The first senrence should describe shortly what the class is.
    2) Please use javadoc (HTML) style formatting.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 225 (original), 225 (patched)
<https://reviews.apache.org/r/65268/#comment276688>

    The first sentence does not explain what this class is doing.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 232 (patched)
<https://reviews.apache.org/r/65268/#comment276690>

    Given exponential nature, how quickly do we usually reach this point?


- Alexander Kolbasov


On Feb. 5, 2018, 3:02 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2018, 3:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/6/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 230 (original), 241 (patched)
<https://reviews.apache.org/r/65268/#comment276691>

    Why do we need sleepTime and newSleepTime?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 243 (patched)
<https://reviews.apache.org/r/65268/#comment276692>

    What is the purpose of this variable?
    Instead, it would be useful to compute the final time right away.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 243 (original), 255 (patched)
<https://reviews.apache.org/r/65268/#comment276694>

    Please use `//` style comments



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 261 (patched)
<https://reviews.apache.org/r/65268/#comment276693>

    We don't want to break here, instead we want to reduce the sleep time to match the remaining balance.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 276 (patched)
<https://reviews.apache.org/r/65268/#comment276695>

    I don't see why you need newSleepTime at all.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 281 (patched)
<https://reviews.apache.org/r/65268/#comment276696>

    Do you need space after `giving up` ?


- Alexander Kolbasov


On Feb. 5, 2018, 3:02 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2018, 3:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/6/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 262 (patched)
<https://reviews.apache.org/r/65268/#comment276710>

    The problem with this is demonstrated by your test. When I run it, I see:
    
    Retried for 283 milli seconds. Giving upInsert 
    
    Your configuration has a limit of 500 ms but you only retried for 283 milliseconds, you could easily try harder.


- Alexander Kolbasov


On Feb. 5, 2018, 3:02 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2018, 3:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/6/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated Feb. 5, 2018, 3:02 p.m.)

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.

Changes
-------

Addressed comments from sasha.

Bugs: SENTRY-1904
https://issues.apache.org/jira/browse/SENTRY-1904

Repository: sentry

Description
-------

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.
3.Removed retry based on count.

With out these limits we would not have a control on how long a transaction could be be active.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/65268/diff/6/

Changes: https://reviews.apache.org/r/65268/diff/5-6/

Testing
-------

Made sure all the tests pass.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated Jan. 31, 2018, 12:30 a.m.)

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.

Changes
-------

Addressed/clarified comments from sasha

Bugs: SENTRY-1904
https://issues.apache.org/jira/browse/SENTRY-1904

Repository: sentry

Description
-------

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.

With out these limits we would not have a control on how long a transaction could be be active.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/65268/diff/5/

Changes: https://reviews.apache.org/r/65268/diff/4-5/

Testing
-------

Made sure all the tests pass.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Lines 249 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950064#file1950064line257>
> >
> >     Do you need extra () here?

based on operator precedence we don't need it but i felt it will be more readable this way.


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Lines 250 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950064#file1950064line258>
> >
> >     Why bvreak - you may adjust sleepTime to be smaller to fit within allotted time.

This code change does 2 things
1. Limit the total trasaction time. (maxRetryDurationMills)
2. Limit the time between retries. (maxRetryTimeIntervalMills)

This break will be hit, if the total trasaction time is exceeded.


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Lines 261 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950064#file1950064line269>
> >
> >     What if this condition is false? In this case you retry immediately without sleeping

Only when the newly calculated sleep time is less than the configured limit, actual sleep time is updated other wise it is ignored and the sleep time will not increse exponentially.
First retry:
sleepTime = 250ms
newSleepTime = 250ms

Second retry:
sleepTime = ~500ms
newSleepTime = 500ms

..
.
.
.
.
N Retry
sleepTime = 25sec
newSleepTime = 60sec

When this happens, sleepTime is not updated with value of newSleepTime as it is greater than maxRetryTimeIntervalMills.


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 93 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950065#file1950065line97>
> >
> >     The comment doesn't match the actual behavior - this is the initial sleep time.

That is what is implemented. Initial sleep time is initialized from retryWaitTimeMills which in-turn is updated from configration SENTRY_STORE_TRANSACTION_RETRY_WAIT_TIME_MILLIS

retryWaitTimeMills = conf.getInt(
        ServerConfig.SENTRY_STORE_TRANSACTION_RETRY_WAIT_TIME_MILLIS,
        ServerConfig.SENTRY_STORE_TRANSACTION_RETRY_WAIT_TIME_MILLIS_DEFAULT);


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 95 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950065#file1950065line99>
> >
> >     The name is confusing. In is not a retry interval but an upper bound of execution for retries.

It is Max limit on time interval between retries.


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 98 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950065#file1950065line102>
> >
> >     Can you express this via Time units?

Please elaborate.


> On Jan. 30, 2018, 1:38 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 103 (patched)
> > <https://reviews.apache.org/r/65268/diff/4/?file=1950065#file1950065line107>
> >
> >     How did you come up with 30 sec and 90 sec numbers?

With the defult retry interval and the exponential increase of the interval it would ~75 sec to complete 10 retries.
The retry interval for the 10th retry would be ~30.

Having the total retry time as 90 sec be similar to what we have now by default. That is why i picked it.

The retry interval for the 10th retry would be ~30. I thought it need not go beyong that. If you have any suggestion, plese let me know.


- kalyan kumar


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


On Jan. 31, 2018, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 31, 2018, 12:30 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/5/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Line 414 (original), 416 (patched)
<https://reviews.apache.org/r/65268/#comment276160>

    This is unrelated change, please remove from the changeset.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 236 (patched)
<https://reviews.apache.org/r/65268/#comment276173>

    Do you need both?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 242 (original)
<https://reviews.apache.org/r/65268/#comment276169>

    It may still be useful to count number of retries and log it. I was often looking for broblems by looking at this log entry.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 249 (patched)
<https://reviews.apache.org/r/65268/#comment276171>

    Do you need extra () here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 250 (patched)
<https://reviews.apache.org/r/65268/#comment276170>

    Why bvreak - you may adjust sleepTime to be smaller to fit within allotted time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 261 (patched)
<https://reviews.apache.org/r/65268/#comment276174>

    What if this condition is false? In this case you retry immediately without sleeping



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 93 (patched)
<https://reviews.apache.org/r/65268/#comment276172>

    The comment doesn't match the actual behavior - this is the initial sleep time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 95 (patched)
<https://reviews.apache.org/r/65268/#comment276162>

    The name is confusing. In is not a retry interval but an upper bound of execution for retries.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 96 (patched)
<https://reviews.apache.org/r/65268/#comment276164>

    Please add time unit in the name.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 98 (patched)
<https://reviews.apache.org/r/65268/#comment276163>

    Can you express this via Time units?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 102 (patched)
<https://reviews.apache.org/r/65268/#comment276165>

    Please add time unit in the name.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 103 (patched)
<https://reviews.apache.org/r/65268/#comment276166>

    How did you come up with 30 sec and 90 sec numbers?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2593 (patched)
<https://reviews.apache.org/r/65268/#comment276168>

    Please add comment explaining what are you testing here. Looks like the name is not telling the truth - you are testing something complicated about notifications.
    
    What you really need is a test that verifies that your failed transactions fail more or less within allotted time, but this test doesn't test for it.


- Alexander Kolbasov


On Jan. 29, 2018, 9:34 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 29, 2018, 9:34 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b984 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b410027 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/4/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated Jan. 29, 2018, 9:34 p.m.)

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.

Changes
-------

Addressed review comments.

Bugs: SENTRY-1904
https://issues.apache.org/jira/browse/SENTRY-1904

Repository: sentry

Description
-------

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.

With out these limits we would not have a control on how long a transaction could be be active.

Diffs (updated)
-----

sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b984
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 91c90f9
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b410027

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

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

Testing
-------

Made sure all the tests pass.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.


> On Jan. 25, 2018, 4:16 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 231 (original), 246 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line246>
> >
> >     If you don't think that you can use time because we do not know how much time it may take to execute transaction, then the whole point of this JIRA is moot and we shouldn't fix it in the first place.

I am not sure if it's the real issue. The two limita are not redundant or mutually exclusive. The old approach limits the number of re-tries; this change also adds limits the total re-try time but keeps the old limitation as well. Are two controls necessarily worse than one? If someone re-configures by mistake retry interval too small (much smaller than the default), so there may be lots of retries (and possibly very unresponsive system) before time limit reached - perhaps we want to prevent it? Can system clock reset at run time messing with the time-only logic? Unlikely, but not impossible.


- Vadim


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


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.


> On Jan. 25, 2018, 4:16 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 231 (original), 246 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line246>
> >
> >     If you don't think that you can use time because we do not know how much time it may take to execute transaction, then the whole point of this JIRA is moot and we shouldn't fix it in the first place.
> 
> Vadim Spector wrote:
>     I am not sure if it's the real issue. The two limita are not redundant or mutually exclusive. The old approach limits the number of re-tries; this change also adds limits the total re-try time but keeps the old limitation as well. Are two controls necessarily worse than one? If someone re-configures by mistake retry interval too small (much smaller than the default), so there may be lots of retries (and possibly very unresponsive system) before time limit reached - perhaps we want to prevent it? Can system clock reset at run time messing with the time-only logic? Unlikely, but not impossible.
> 
> kalyan kumar kalvagadda wrote:
>     Vadim, Having both limits on retry-count and the retry-time would address the scenario you are taking about.

Kalyan, that was exactly my point, sorry if it was not clear. I support keeping both mechanisms in place, as in your patch.


- Vadim


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


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On Jan. 25, 2018, 4:16 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 231 (original), 246 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line246>
> >
> >     If you don't think that you can use time because we do not know how much time it may take to execute transaction, then the whole point of this JIRA is moot and we shouldn't fix it in the first place.
> 
> Vadim Spector wrote:
>     I am not sure if it's the real issue. The two limita are not redundant or mutually exclusive. The old approach limits the number of re-tries; this change also adds limits the total re-try time but keeps the old limitation as well. Are two controls necessarily worse than one? If someone re-configures by mistake retry interval too small (much smaller than the default), so there may be lots of retries (and possibly very unresponsive system) before time limit reached - perhaps we want to prevent it? Can system clock reset at run time messing with the time-only logic? Unlikely, but not impossible.

Vadim, Having both limits on retry-count and the retry-time would address the scenario you are taking about.


- kalyan kumar


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


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 246 (patched)
<https://reviews.apache.org/r/65268/#comment275813>

    If you don't think that you can use time because we do not know how much time it may take to execute transaction, then the whole point of this JIRA is moot and we shouldn't fix it in the first place.


- Alexander Kolbasov


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.


> On Jan. 25, 2018, 12:35 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 231 (original), 246 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line246>
> >
> >     Since you are switching to time-based max it becomes the hard line. You want to either limit number of retries or total number but not both at the same time.

Configuration to limit the retry count is already avaialable in the released versions of sentry. It's not a good idea to remove it.

We do not know what is the time that would take for a transaction to be succsfull in the event of JDO failure that is why time based limit is set to some higher number so that it will not be hit in normal cases unless some configured ridiculously high retry count or has configured high retry time.


> On Jan. 25, 2018, 12:35 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
> > Line 247 (original), 268 (patched)
> > <https://reviews.apache.org/r/65268/diff/3/?file=1944703#file1944703line268>
> >
> >     This may make total sleep time greater then the allowed sleep time. You need to make sure that you never sleep longer then the remaining time left.

In each iteration sleep time is calculated for next iteration. Logic added at line 255 will make sure that the total sleep time never exceeds max allowed sleep time.


- kalyan kumar


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


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




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

    The JIRA is about transaction retry logic and this mixes it with actual uses - it doesn't belong here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 246 (patched)
<https://reviews.apache.org/r/65268/#comment275733>

    Since you are switching to time-based max it becomes the hard line. You want to either limit number of retries or total number but not both at the same time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 247 (original), 268 (patched)
<https://reviews.apache.org/r/65268/#comment275734>

    This may make total sleep time greater then the allowed sleep time. You need to make sure that you never sleep longer then the remaining time left.


- Alexander Kolbasov


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 11:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

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

(Updated Jan. 23, 2018, 11:40 p.m.)


Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Changes
-------

Addressed review comment from vadim


Bugs: SENTRY-1904
    https://issues.apache.org/jira/browse/SENTRY-1904


Repository: sentry


Description
-------

The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.
 

With out these limits we would not have a control on how long a transaction could be be active.


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 


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

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


Testing
-------

Made sure all the tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Lines 421 (patched)
<https://reviews.apache.org/r/65268/#comment275594>

    Since this code is inside catch {} and serves the purpose of clarifying the possible exception cause, would it make sense to put it inside its own try-catch so if the exception is thrown, it would be logged, not re-thrown from the parent catch {} case?


- Vadim Spector


On Jan. 23, 2018, 6:43 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 6:43 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/2/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by Na Li via Review Board <no...@reviews.apache.org>.

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


Ship it!




Ship It!

- Na Li


On Jan. 23, 2018, 6:43 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 6:43 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1904
>     https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> The TransactionManager uses exponential backoff strategy for transaction retries. This may cause some transactions to be delayed by a very long time. We should also have a constraint on the max time for a transaction so that we do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
>  
> 
> With out these limits we would not have a control on how long a transaction could be be active.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/2/
> 
> 
> Testing
> -------
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

(Updated Jan. 23, 2018, 6:43 p.m.)

Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.

Changes
-------

Changed the solution approach.

Bugs: SENTRY-1904
https://issues.apache.org/jira/browse/SENTRY-1904

Repository: sentry

Description (updated)
-------

New patch that is attached adds upper bounds on below

1.Interval between the retry attempts which increases exponentially.
2.Total time a transaction could spend in retries.

With out these limits we would not have a control on how long a transaction could be be active.

Diffs (updated)
-----

sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 2f2b98412e7dfdcc847ffe7975a70f452554e747
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java edea5b64d8f98c93aafc1fe43fa97e00c2ce2948
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b4be6a7109108809b1f404fe971b7b8e2
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b4100278392986c161625a366212c6fef66ec0a9

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

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

Testing
-------

Made sure all the tests pass.

Thanks,

kalyan kumar kalvagadda