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