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

[Alexander Kolbasov]

---
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)


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

[Alexander Kolbasov]

---
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)


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)


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)


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)


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)


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)


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)


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

[Alexander Kolbasov]

---
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)


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)


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)


Please use `//` style comments



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 261 (patched)


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)


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)


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

[Alexander Kolbasov]

---
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)


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)


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)


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: Please welcome Kalyan Kalvagadda to Apache PMC!

[Hao Hao]

Congrats!

Best,
Hao

On Mon, Feb 5, 2018 at 3:18 PM, Stephen Moist  wrote:

> Congrats!
>
> > On Feb 5, 2018, at 12:33 PM, Na Li  wrote:
> >
> > Kalyan, Congratulation!
> >
> > On Mon, Feb 5, 2018 at 2:18 PM, Sergio Pena 
> > wrote:
> >
> >> Congratulations Kalyan !!!
> >>
> >> On Mon, Feb 5, 2018 at 2:16 PM, Xinran Yu Tinney <
> yuxinran8...@gmail.com>
> >> wrote:
> >>
> >>> Congratulations!
> >>>
> >>> 2018-02-05 14:14 GMT-06:00 Alexander Kolbasov :
> >>>
>  Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!
> 
>  - Alex
> 
> >>>
> >>
>
>

Re: Please welcome Kalyan Kalvagadda to Apache PMC!

[Stephen Moist]

Congrats!

> On Feb 5, 2018, at 12:33 PM, Na Li  wrote:
> 
> Kalyan, Congratulation!
> 
> On Mon, Feb 5, 2018 at 2:18 PM, Sergio Pena 
> wrote:
> 
>> Congratulations Kalyan !!!
>> 
>> On Mon, Feb 5, 2018 at 2:16 PM, Xinran Yu Tinney 
>> wrote:
>> 
>>> Congratulations!
>>> 
>>> 2018-02-05 14:14 GMT-06:00 Alexander Kolbasov :
>>> 
 Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!
 
 - Alex
 
>>> 
>> 

Re: Please welcome Kalyan Kalvagadda to Apache PMC!

[Na Li]

Kalyan, Congratulation!

On Mon, Feb 5, 2018 at 2:18 PM, Sergio Pena 
wrote:

> Congratulations Kalyan !!!
>
> On Mon, Feb 5, 2018 at 2:16 PM, Xinran Yu Tinney 
> wrote:
>
> > Congratulations!
> >
> > 2018-02-05 14:14 GMT-06:00 Alexander Kolbasov :
> >
> > > Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!
> > >
> > > - Alex
> > >
> >
>

Re: Please welcome new Apache Sentry committer Na Li

[Sergio Pena]

Congratulations Lina !!!

On Mon, Feb 5, 2018 at 2:05 PM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> Hey Lina,
>
>
> Congratulations.
>
>
> -Kalyan
>
> On Mon, Feb 5, 2018 at 2:04 PM, Alexander Kolbasov 
> wrote:
>
> > Hi Sentry developers, please welcome Na Li who is a new committer on the
> > Apache Sentry Project!
> >
> > Congratulations!
> >
> > - Alex
> >
>

Re: Please welcome Kalyan Kalvagadda to Apache PMC!

[Sergio Pena]

Congratulations Kalyan !!!

On Mon, Feb 5, 2018 at 2:16 PM, Xinran Yu Tinney 
wrote:

> Congratulations!
>
> 2018-02-05 14:14 GMT-06:00 Alexander Kolbasov :
>
> > Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!
> >
> > - Alex
> >
>

Re: Please welcome Kalyan Kalvagadda to Apache PMC!

[Xinran Yu Tinney]

Congratulations!

2018-02-05 14:14 GMT-06:00 Alexander Kolbasov :

> Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!
>
> - Alex
>

Please welcome Kalyan Kalvagadda to Apache PMC!

[Alexander Kolbasov]

Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!

- Alex

Re: Please welcome new Apache Sentry committer Na Li

[Kalyan Kumar Kalvagadda]

Hey Lina,


Congratulations.


-Kalyan

On Mon, Feb 5, 2018 at 2:04 PM, Alexander Kolbasov 
wrote:

> Hi Sentry developers, please welcome Na Li who is a new committer on the
> Apache Sentry Project!
>
> Congratulations!
>
> - Alex
>

Re: Please welcome new Apache Sentry committer Na Li

[Xinran Yu Tinney]

Congratulations!

2018-02-05 14:04 GMT-06:00 Alexander Kolbasov :

> Hi Sentry developers, please welcome Na Li who is a new committer on the
> Apache Sentry Project!
>
> Congratulations!
>
> - Alex
>

Please welcome new Apache Sentry committer Na Li

[Alexander Kolbasov]

Hi Sentry developers, please welcome Na Li who is a new committer on the
Apache Sentry Project!

Congratulations!

- Alex

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

[kalyan kumar kalvagadda via Review Board]

---
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.


Changes
---

Addressed comments from sasha.


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/6/

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


Testing
---

Made sure all the tests pass.


Thanks,

kalyan kumar kalvagadda