You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2018/02/02 00:57:44 UTC
Re: 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/#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
>
>