> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1952977#file1952977line138> > > > > 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.
will update > On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3424 (patched) > > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3424> > > > > Can we use something simpler to test this? What would be the simplest > > thing that we can use to cause exceptions? I thought simpler way to simulate this failure was by inserting duplicate entries. That is what I have done. Let me know of you can think anything simpler > On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3435 (patched) > > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3435> > > > > This can be just `new HashMap<>()` will change. > On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3447 (patched) > > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3447> > > > > It would be better to simplify the message - e.g. > > `fail("expected JDODataStoreException")` will change. > On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3449 (patched) > > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3449> > > > > Do you know that it is JDODataStoreException? WIll it be the same if > > the test is executed on non-Derby DB? Performing a duplicate insert would result in JDODataStoreException(derived from javax.jdo.JDOCanRetryException) from datanucleus. This exception is not specific to derby and should be seen even for other databases. Sentry store tries to catch the same exception in many API's. > On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3450 (patched) > > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3450> > > > > Unexpected failure isn't very useful - it is better to describe what > > went wrong - what you expected and what happened instead. Will update. > On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3453 (patched) > > <https://reviews.apache.org/r/65268/diff/6/?file=1952977#file1952977line3453> > > > > millisecond is one word Will update. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65268/#review196849 ----------------------------------------------------------- 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 > >
