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