Allow clients to request the transactor to retry queries by okulkarni · Pull Request #261 · LiveRamp/jack

Omkar Kulkarni (okulkarni) · 2019-09-24T17:37:46Z

This patch is inspired by #210 - It introduces ExecutionContext that allows clients to specify options for every query/update. Currently the two options supported are to run as a transaction (deprecates queryAsTransaction and executeAsTransaction methods) and set a retry policy on failure. The default behavior remains unchanged, namely, no retries on failure. However, clients can now use the new exponential backoff policy with tunable number of retries and sleep intervals. Users could potentially create their own advanced retry policies by implementing the ITransactor.RetryPolicy interface. The policy is executed on failure only after the connection has been invalidated or returned to the pool manager.

Example:

transactor
  .asTransaction()
  .allowRetries(new ExponentialBackoffRetryPolicy(numRetries, sleepInterval))
  .query(myQuery);

https://liveramp.atlassian.net/browse/DIST-2702

See DIST-2702

Davin Chia (davinchia)

Nice PR! Good tests! Spoke offline - one change before I think we are good to go.

Davin Chia (davinchia) · 2019-09-26T23:11:54Z

+  private int retryInterval = 0;
+  private boolean succeeded = false;
+
+  public ExponentialBackoffRetryPolicy() {


As per https://en.wikipedia.org/wiki/Principle_of_least_astonishment, i'd expect the default constructor of a class named ExponentialBackoffRetryPolicy to have a default non-null value. What about having default static constants for these instead?

I also see we are using this class to implement the NoOp retry policy - what about having a ConfigurableRetryPolicy class with static methods that return a no-op policy and a default exponential policy?

Davin Chia (davinchia) · 2019-09-27T01:41:58Z

+    }
+  }
+
+  class NoRetryPolicy extends ExponentialBackoffRetryPolicy {}


If we go with the above implementation of base retry class + static builder method, NoRetryPolicy no longer needs to exists.

Davin Chia (davinchia) · 2019-09-27T01:42:12Z

+  }
+
+  public ExponentialBackoffRetryPolicy(int maxRetries) {
+    this(maxRetries, new Random().nextInt(5000) + 10000);


What do you think about letting others configure this backoff? A default definitely makes sense, but since we are enabling this, what about letting others configure the intial backoff period + an exponential multiplier for each successive backoff?

We'd have an additional field named backoffMultiplier the constructor would take in. This would also allow someone to configure a linear backoff without any code changes at all (for whatever reason one would want one instead of exponential).

Davin Chia (davinchia) · 2019-09-27T01:43:10Z

+    if (shouldRetry()) {
+      LOG.warn("Retry #" + numFailures + ": Going to sleep for " + retryInterval + " milliseconds.");
+      sleep(retryInterval);
+      retryInterval <<= 1;


Whoa been a while since i've seen bitshift code - fancy C stuff here!

Davin Chia (davinchia) · 2019-09-27T01:43:17Z

  public void testWaitForConnectionTimeout() throws Exception {
-    TransactorImpl<IDatabase1> transactor = transactorBuilder.setMaxTotalConnections(1).setMaxWaitTime(Duration.ofMillis(500)).get();
+    TransactorImpl<IDatabase1> transactor =
+        transactorBuilder.setMaxTotalConnections(1).setMaxWaitTime(Duration.ofMillis(500)).get();


🙏 for the reformatting

The IDE did it.

Omkar Kulkarni (okulkarni)

Agree with most comments. However, decided to keep ExponentialBackoffRetryPolicy as is while removing the "no retry" case (and eliminating astonishment), and instead of introducing more complexity with a new base class and builder methods, decided to simply refactor NoRetryPolicy as essentially a no-op policy which always returns false for shouldRetry.

Omkar Kulkarni (okulkarni) · 2019-09-30T17:19:00Z

  public void testWaitForConnectionTimeout() throws Exception {
-    TransactorImpl<IDatabase1> transactor = transactorBuilder.setMaxTotalConnections(1).setMaxWaitTime(Duration.ofMillis(500)).get();
+    TransactorImpl<IDatabase1> transactor =
+        transactorBuilder.setMaxTotalConnections(1).setMaxWaitTime(Duration.ofMillis(500)).get();


The IDE did it.

Davin Chia (davinchia)

lgtm apart from 1 nit

Davin Chia (davinchia) · 2019-09-30T22:00:57Z

+        } else {
+          dbManager.invalidateConnection(connection);
+        }
+        context.retryPolicy.execute();


nit: comment explaining why we want to retry after returning/invalidating connections

Brian Ecker (briancecker)

Nice PR. I like the way you approached this with the ExecutionContext.

Brian Ecker (briancecker) · 2019-09-30T20:34:03Z

+
+    boolean shouldRetry();
+
+    void updateOnFailure();


Could you document the purpose of this method or make the name more clear? I had a hard time understanding what it was used for until I saw the exponential implementation. I feel like just onFailure would be a better name. The update part really confused me, and onFailure/onSuccess are relatively common names for callback type methods.

Does it make sense to provide some sort of argument to these methods? For instance, onFailure could take an Exception as an argument to allow the implementing class to modify retry behavior based on the type of failure. You could then differentiate between retrying on a transient exception and not retrying on a logical exception.

Agree completely on the first part. I had originally named it onFailure and onSuccess, I am not sure why I changed that. :-/ I realize that on{EventName} is common for handling events (My Java is still a bit rusty after having worked on C all this while).

Probably a good idea to allow onFailure to handle the exception -- which makes me think that the shouldRetry method is redundant since onFailure could simply throw/propagate the exception to exit the loop. Thoughts?

Hmm, that's also another way of looking at it. So the usage would be like the following, correct?

try to execute a query

catch an exception

in the catch block, call onFailure(e)

if the number of retries is below a certain number, then you increment the number and execute what is now in the execute method

If the number of retries is above the threshold, then you rethrow the exception allowing the method to fail normally

Maybe this whole thing could just be onFailure? What do you think?

That was the original thought; However, the reason I have onSuccess and execute is to allow the policy to sleep (or do some other blocking/long running task) after returning the connection to the connection pool, which is located in the finally block. Since the finally block executes no matter what, there must be some way of knowing when not to sleep -- either the number of retries have been exhausted or the query was successful.

1. Removed `shouldRetry` (redundant). 2. Renamed to `onSuccess` and `onFailure` for clarity. 3. `onFailure` now takes an exception as an argument. 4. `execute` returns boolean indicating whether policy was executed.

Comment to explain why policy is executed after returning/invalidating connection.

Omkar Kulkarni (okulkarni) requested review from Brian Ecker (briancecker), Edward Gao (edgao) and Sherif A. Nada (sherifnada) September 24, 2019 17:43

Allow clients to request the transactor to retry queries

f9b8de9

See DIST-2702

Omkar Kulkarni (okulkarni) force-pushed the retry_queries branch from fdb4b59 to f9b8de9 Compare September 26, 2019 16:27

Davin Chia (davinchia) suggested changes Sep 27, 2019

View reviewed changes

Omkar Kulkarni added 2 commits September 30, 2019 10:15

Addressing review comments.

92527dd

Make ExecutionContext private.

b6e77a2

Omkar Kulkarni (okulkarni) commented Sep 30, 2019

View reviewed changes

Davin Chia (davinchia) mentioned this pull request Sep 30, 2019

Allow clients to request the transactor to retry queries #210

Closed

Davin Chia (davinchia) approved these changes Sep 30, 2019

View reviewed changes

Brian Ecker (briancecker) suggested changes Sep 30, 2019

View reviewed changes

Corrected test.

5dd9e62

Omkar Kulkarni (okulkarni) force-pushed the retry_queries branch from 855d0c3 to 5dd9e62 Compare September 30, 2019 22:39

Omkar Kulkarni added 2 commits October 1, 2019 09:03

Addressed some comments:

355763b

1. Removed `shouldRetry` (redundant). 2. Renamed to `onSuccess` and `onFailure` for clarity. 3. `onFailure` now takes an exception as an argument. 4. `execute` returns boolean indicating whether policy was executed.

Added comment

3cc0b77

Comment to explain why policy is executed after returning/invalidating connection.

Omkar Kulkarni (okulkarni) requested a review from Brian Ecker (briancecker) October 1, 2019 21:07

Brian Ecker (briancecker) approved these changes Oct 1, 2019

View reviewed changes

Omkar Kulkarni (okulkarni) merged commit 7216a66 into master Oct 1, 2019

delete-merged-branch Bot deleted the retry_queries branch October 1, 2019 23:24

Uh oh!

Conversation

Omkar Kulkarni (okulkarni) commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Davin Chia (davinchia) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omkar Kulkarni (okulkarni) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davin Chia (davinchia) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brian Ecker (briancecker) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Omkar Kulkarni (okulkarni) commented Sep 24, 2019 •

edited

Loading