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