Add retry logic to requests #33

Joneser · 2017-10-04T06:09:09Z

This is a first PR to get feedback on my approach

Uses the retry library to wrap the promises as retriable
Add configurable function in order to perform transient error checks
Improve logging/tracking of retried requests
Add tests for retry logic

Fixes #18

shuhei · 2017-10-04T07:27:04Z

Nice work! I have a couple of questions:

How will this interact with the circuit breaker? I expect one attempt to be counted as one call in CB. For example, a fault tolerant request that failed twice and made it with the third attempt should be counted as 2 failures and 1 success in CB.
Do we need an additional public method for fault tolerant requests? I think the existing request method can handle retries using the retry count.

shuhei · 2017-10-04T15:16:50Z

Also:

Shouldn't this provide a way for users to specify which errors are transient?
Should exiting options like dropRequestAfter be affected by retry?

Joneser · 2017-10-09T08:44:59Z

@shuhei

With this implementation it will behave as you expect, if it fails twice and the third attempt succeeds, this will count as 2 failures and 1 success.
Due to the use of the retry logic, I thought it would be easiest to just use a separate method instead of adding additional logic into the existing function. This also has the benefit of not affecting the existing users (not that it should, but just to be safe)
I will add configuration options for the transient error configuration today.
To be honest I'm not sure, I'll also investigate this today.

grassator · 2017-10-09T10:28:29Z

@Joneser regarding 2. I'm also with @shuhei — you can split the logic internally, but strive to keep a simple external API. Having a default retry count of 1 should result in the same behavior as we currently have.

Joneser · 2017-10-09T10:50:27Z

@grassator @shuhei No problem, I'll move it into the same function while I am adding the transient error configuration

Joneser · 2017-10-11T07:12:33Z

@shuhei @grassator I have now moved the retry logic into the same function and have defaulted the config to 0 retries, so that it behaves exactly as it did before.

For the transient error configuration, I decided on a function that can be passed in by the consumer to allow them to customise the configuration any way they want. I thought that would be easier than expecting lists of status codes or error strings. Perhaps this should just be a noop by default instead of what I have there?

Does anyone have suggestions on how to approach the logging of the retries? Should we allow the user to pass in their own logger which will get called or some kind of aggregator? Perhaps we should pass the retry count for a request in the response/error object the same way as the timings?

grassator · 2017-10-11T12:36:16Z

test/client.test.js

@@ -1,5 +1,7 @@
 'use strict';

+/* eslint-disable */


Is it possible to disable only a certain check and not whole eslint?

Woops my bad, missed this!

grassator · 2017-10-11T12:37:49Z

README.md

+Occasionaly target server can have high latency for a short period of time, or in the case of a stack of servers, one server can be having issues
+and retrying the request will allow perron to attempt to access one of the other servers that currently aren't facing issues.
+
+By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic


probably missing "uses" after "Internally perron"

grassator · 2017-10-11T12:42:55Z

README.md

+const catWatch = new ServiceClient({
+    hostname: 'catwatch.opensource.zalan.do',
+    // These are the default settings
+    retryOptions: {


I think that README should provide useful example options, since that is what users will copy-paste by default. If I read node-retry documentation formula right, current config will result in a not linear distribution between 200 and 300ms with more than half delays being 300ms.

Any recommendations as to what you would prefer here? Currently this example is the same as what the default retryOptions are configured in perron (but with retries set to 1), so I should also update there if you believe that the current config is not appropriate

I would just change maxTimeout to 400 probably

Ok no worries, I'll update :)

grassator · 2017-10-11T12:44:29Z

lib/client.js

+            randomize: true,
+            // this is the default function to check transient errors
+            // users should define their own check in order to handle errors as they require
+            transientErrorCheck: (err) => {


Why the default function shouldn't be just () => true ?

grassator · 2017-10-11T12:45:32Z

README.md

+        minTimeout: 200,
+        maxTimeout: 300,
+        randomize: true,
+        transientErrorCheck: (err) => {


You don't need an arrow function here:

transientErrorCheck(err) {

grassator · 2017-10-12T08:23:41Z

lib/client.js

@@ -248,13 +273,27 @@ class ServiceClient {

        return new Promise((resolve, reject) => this.breaker.run(


It looks like my comment got lost somehow, but here I'm a little worried that you will call failure callback multiple times inside a single breaker.run call, which doesn't seem logically right and also might cause issues. Can we change that so each retry is also a breaker.run?

Sure, I'll look into restructuring it this afternoon.
Any thoughts on how we can provide information on the number of retries performed to the user of perron? I was thinking of possibly adding a value to the response/error object

It might be OK, but I'm not sure so far what is the best way to provide information about each stage of the request, would be open to any ideas, as now error / info reporting is not really great in perron at the moment

After discussing with @shuhei I will add a callback function like onRetry that the consumer of perron can define in order to track the retry action how they like

Joneser · 2017-10-16T12:17:52Z

lib/client.js

+            // it takes the error as an argument
+            transientErrorCheck(err) {
+                // by default don't retry if the circuit breaker is open
+                if (err.type === ServiceClient.CIRCUIT_OPEN) {


I thought this would be a good idea to have by default, but again if you just want it to return true by default I can remove this

How about renaming this as shouldRetry? Open circuit breaker is also literally transient.

In relation to this naming, it would conflict with here: https://github.com/zalando-incubator/perron/pull/33/files#diff-50cfa59973c04321b5da0c6da0fdf4feR314

What would your naming preference be for the already defined shouldRetry

The existing shouldRetry is just an internal variable, which matters less than public API. Now I think that the existing one is a result of retry module's decision, which may have already triggered a retry. So how about retrying, retryingNext or willRetry?

Good point, will update

shuhei · 2017-10-16T14:05:19Z

lib/client.js

+ *      minTimeout?: number,
+ *      maxTimeout?: number,
+ *      randomize?: boolean
+ *  },


It would be nice to have transientErrorCheck and onRetry here.

shuhei · 2017-10-16T14:07:29Z

lib/client.js

+        }, this.options.retryOptions);
+
+        if (this.options.retryOptions.minTimeout > this.options.retryOptions.maxTimeout) {
+            throw new Error('The `retryInterval` must be equal to or greater than the `minTimeout`');


maxTimeout intead of retryInterval?

shuhei · 2017-10-16T14:14:47Z

lib/client.js

+                            reject(error);
+                            return;
+                        }
+                        onRetry(currentAttempt, error);


Shouldn't it be currentAttempt + 1? The retry will be the next attempt.

The first time onRetry is called, currentAttempt is 1 which I think is correct

shuhei · 2017-10-16T14:15:06Z

lib/client.js

+            // it receives the current retry attempt and error as args
+            // eslint-disable-next-line
+            onRetry(currentAttempt, err) {
+                return currentAttempt;


Why do you return this?

It is just a default placeholder, do you think an empty function would be better?

Yes, if we return something here, people will think that they should also return something.

shuhei · 2017-10-16T14:16:46Z

README.md

+attempt the retries or not depending on the type of error. If the function returns true and the number of retries hasn't been exceeded, the request can be retried.
+
+There is also an onRetry function which can be defined by the user of `perron`. This function is called every time a retry request will be triggered.
+It is provided the currentAttempt, as well as the error that is causing the retry.


It would be nice to refer to what currentAttempt means. Is it 0 or 1 for the first retry?

The first time onRetry is triggered it is 1, I'll update the readme with more info.

shuhei · 2017-10-16T14:19:34Z

test/client.test.js

+    it('should perform the desired number of retries based on the configuration', (done) => {
+        let numberOfRetries = 0;
+        clientOptions.retryOptions = {
+            retries: 1,


I'd put 2 or more just to make sure that it retries multiple times.

shuhei · 2017-10-16T14:26:24Z

test/client.test.js

+            assert.equal(err.type, ServiceClient.CIRCUIT_OPEN);
+            done();
+        });
+    });


How about adding a test for the transient error function?

shuhei · 2017-10-16T15:36:05Z

lib/client.js

                    })
                    .catch(error => {
-                        failure(); reject(error);
+                        failure();
+                        const shouldRetry = operation.retry(error);


Ohh, by the way, this is already triggering retry! This should come after the transient error check.

Maybe something like this?

const canRetry = currentAttempt > retries; if (!shouldRetry(error) || !canRetry) { reject(error); return; } if(operation.retry(error)) { onRetry(currentAttempt, error); }

Or this probably makes more sense:

if (!shouldRetry(error)) { reject(error); return; } if(!operation.retry(error)) { reject(error); return; } onRetry(currentAttempt, error);

Yeah, the second looks good to me.

grassator · 2017-10-17T11:05:37Z

README.md

+        minTimeout: 200,
+        maxTimeout: 400,
+        randomize: true,
+        shouldRetry(err) {


Thinking about it, just the error might not be enough. You might need info from original request as well to make a judgement (e.g. only retry GET requests)

Good point, I'll add the original request info as well

Joneser · 2017-10-17T11:51:03Z

@grassator @shuhei Ready for the next review :)

grassator · 2017-10-17T14:25:13Z

👍

shuhei · 2017-10-17T14:28:37Z

👍

grassator · 2017-10-17T15:37:37Z

@Joneser Thank you for the contribution! I released 0.4.0-beta1 to NPM

Joneser · 2017-10-17T17:00:29Z

Thanks a million @grassator and @shuhei for the reviews!

djones and others added 2 commits October 2, 2017 09:11

adding retry logic

c436c84

test updates

56606fa

add retry to request function and transient error config

6bff0ca

update readme

8329ddb

Joneser changed the title ~~Add fault tolerant (retriable) requests~~ Add retry logic to requests Oct 11, 2017

grassator reviewed Oct 11, 2017

View reviewed changes

cleaning up pr comments

a987bdf

grassator reviewed Oct 12, 2017

View reviewed changes

update maxTimeout

e6ef982

Joneser commented Oct 16, 2017

View reviewed changes

add retry callback and initial test

c50870c

shuhei reviewed Oct 16, 2017

View reviewed changes

some cleanup based on PR comments

3dcacfd

grassator reviewed Oct 17, 2017

View reviewed changes

check shouldRetry before operation.retry

bbc58dd

grassator merged commit fd9a203 into zalando-incubator:master Oct 17, 2017

		@@ -248,13 +273,27 @@ class ServiceClient {

		return new Promise((resolve, reject) => this.breaker.run(

Add retry logic to requests #33

Add retry logic to requests #33

Conversation

Joneser commented Oct 4, 2017 • edited by grassator

shuhei commented Oct 4, 2017 • edited

shuhei commented Oct 4, 2017

Joneser commented Oct 9, 2017

grassator commented Oct 9, 2017

Joneser commented Oct 9, 2017

Joneser commented Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joneser Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joneser Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhei Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhei Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhei Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joneser Oct 17, 2017 • edited

Choose a reason for hiding this comment

Joneser Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joneser Oct 17, 2017 • edited

Choose a reason for hiding this comment

Joneser commented Oct 17, 2017

grassator commented Oct 17, 2017

shuhei commented Oct 17, 2017

grassator commented Oct 17, 2017

Joneser commented Oct 17, 2017

Joneser commented Oct 4, 2017 •

edited by grassator

shuhei commented Oct 4, 2017 •

edited

Joneser commented Oct 11, 2017 •

edited

Joneser Oct 11, 2017 •

edited

Joneser Oct 12, 2017 •

edited

shuhei Oct 16, 2017 •

edited

shuhei Oct 16, 2017 •

edited

shuhei Oct 16, 2017 •

edited

Joneser Oct 17, 2017 •

edited

Joneser Oct 17, 2017 •

edited

Joneser Oct 17, 2017 •

edited