Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Retrying on any error from the Watch stream #70

Merged
merged 6 commits into from
Nov 16, 2017
Merged

Retrying on any error from the Watch stream #70

merged 6 commits into from
Nov 16, 2017

Conversation

schmidt-sebastian
Copy link
Contributor

This ports the exponential backoff logic from the Firestore Mobile Web client.

Fixes firebase/firebase-admin-node#119

@schmidt-sebastian
Retrying on any error for Watch stream.
0002a39 
This ports the exponential backoff logic from the Firestore Mobile Web client.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 14, 2017
@schmidt-sebastian schmidt-sebastian changed the title Retrying on any error for Watch stream. Retrying on any error from the Watch stream Nov 14, 2017
@codecov
Copy link

codecov bot commented Nov 14, 2017 •
edited
Loading

Codecov Report

Merging #70 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #70   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     12    +1     
  Lines        1481   1524   +43     
=====================================
+ Hits         1481   1524   +43
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️
src/backoff.js 100% <100%> (ø)
src/watch.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acae581...a8a605c. Read the comment docs.

mikelehen
mikelehen suggested changes Nov 15, 2017
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random nits as per usual. :-)

src/backoff.js Outdated
const BACKOFF_FACTOR = 1.5;

/*!
* The jitter to distribute the backoff attempts by.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/backoff.js Outdated
*/

/*!
* Initial backoff time in milliseconds after an error.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/backoff.js Outdated
const BACKOFF_INITIAL_DELAY_MS = 1000;

/*!
* Maximum backoff time in milliseconds.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/backoff.js Outdated
const BACKOFF_MAX_DELAY_MS = 60 * 1000;

/*!
* The factor to increase the backup by after each failed attempt.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/backoff.js Outdated
* This corresponds to
* {@link https://github.com/grpc/grpc/blob/master/doc/statuscodes.md}.
*/
const GRPC_STATUS_CODE = {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/backoff.js

function assertDelayBetween(low, high) {
let actual = observedDelays.shift();
assert.ok(actual >= low);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/watch.js Outdated
@@ -591,6 +597,8 @@ describe('Query watch', function() {
};

beforeEach(function() {
Backoff.setTimeoutHandler(setImmediate);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/watch.js Outdated
watchHelper.startWatch();
it("doesn't re-open inactive stream", function() {
// This test uses the normal timeout handler since we unsubscribe from the
// Watch stream while the stream is recovering from an error.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/watch.js Outdated
@@ -986,7 +1058,7 @@ describe('Query watch', function() {
streamHelper.writeStream.destroy();
});
},
'Error: Steam Error (6)'
'Steam Error (6)'

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/watch.js Outdated
@@ -1809,13 +1881,19 @@ describe('DocumentReference watch', function() {
};

beforeEach(function() {
Backoff.setTimeoutHandler(setImmediate);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@schmidt-sebastian
Copy link
Contributor Author

Addressed all comments. Thanks @mikelehen

@mikelehen
Copy link
Contributor

LGTM. Sorry for the delay.

@schmidt-sebastian
Copy link
Contributor Author

@stephenplusplus Can you also look at this before I merge this? This adds retry semantics for Watch.

@stephenplusplus
Copy link
Contributor

LGTM.

@schmidt-sebastian schmidt-sebastian merged commit fc55e36 into master Nov 16, 2017
@ghost ghost removed the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2017
This was referenced Nov 16, 2017
Closed
Merged
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt-backoff branch January 30, 2018 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikelehen mikelehen mikelehen approved these changes

Assignees

@stephenplusplus stephenplusplus

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schmidt-sebastian @mikelehen @stephenplusplus @googlebot