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

feat: use GAX retry config for streams #847

Merged
merged 17 commits into from
Jan 2, 2020
Merged

feat: use GAX retry config for streams #847

merged 17 commits into from
Jan 2, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

This PR cleans up our request handling a little bit:

  • It removes our internal request retry mechanism for "normal" requests and instead relies on Google Gax to do the retries.
  • It changes the retry logic for streaming requests to use the error codes defined in the Service Config (as far as I am aware, Gax does not retry streaming requests)
  • It changes the backoff timer for streaming requests to use the backoff timer that Firestore is already using for Watch.
  • It drops our own error class in favor of Google Gax's GoogleError.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 30, 2019
schmidt-sebastian
schmidt-sebastian commented Dec 30, 2019
View reviewed changes
dev/src/types.ts Outdated Show resolved Hide resolved
dev/test/transaction.ts
@@ -457,21 +459,6 @@ describe('failed transactions', () => {
).to.eventually.be.rejectedWith('Final exception');
});

it('fails on beginTransaction', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test no longer works, since the retries are handled in google-gax, which is replaced in the unit tests with dumb implementation that doesn't retry.

@codecov
Copy link

codecov bot commented Dec 30, 2019 •
edited
Loading

Codecov Report

Merging #847 into master will decrease coverage by 1.29%.
The diff coverage is 98.44%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #847     +/-   ##
========================================
- Coverage   88.69%   87.4%   -1.3%     
========================================
  Files          27      27             
  Lines       16681   16542    -139     
  Branches     1151    1151             
========================================
- Hits        14796   14458    -338     
- Misses       1880    2078    +198     
- Partials        5       6      +1
Impacted Files Coverage Δ
dev/src/types.ts 0% <ø> (-100%) ⬇️
dev/src/write-batch.ts 99.57% <100%> (-0.01%) ⬇️
dev/src/backoff.ts 100% <100%> (ø) ⬆️
dev/src/util.ts 100% <100%> (ø) ⬆️
dev/src/watch.ts 98.22% <100%> (-0.23%) ⬇️
dev/src/reference.ts 99.07% <100%> (-0.01%) ⬇️
dev/src/transaction.ts 96.66% <100%> (-0.08%) ⬇️
dev/src/index.ts 98.7% <97.36%> (-0.09%) ⬇️

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 2ad8acc...28a9206. Read the comment docs.

@schmidt-sebastian schmidt-sebastian changed the base branch from master to mrschmidt/removebun December 31, 2019 00:43
alexander-fenster
alexander-fenster reviewed Dec 31, 2019
View reviewed changes
dev/src/index.ts Outdated
}

try {
const result = await backoff.backoffAndWait().then(func);
Copy link
Contributor

@alexander-fenster alexander-fenster Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Mixing await and then in the same line is kind of confusing. Is it the same as the following code?

await backoff.backoffAndWait();
this._lastSuccessfulRequest = new Date().getTime();
return await func();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not quite (the successful request should only be set after await func()). I cleaned it up.

alexander-fenster
alexander-fenster reviewed Dec 31, 2019
View reviewed changes
dev/src/util.ts Outdated
methodName: string,
config: ClientConfig
): boolean {
const serviceConfig = config.interfaces!['google.firestore.v1.Firestore']!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up, this config file will go away soon. By "soon" here I mean half-a-year-ish time. When backend starts supporting returning this configuration via DNS (TXT record I guess), we'll start using it and will stop generating those JSON configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I will find some clever way to get this configuration in 6 months from now.

Or rather - I will find a clever way to push this code into GAX.

@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/removebun to master December 31, 2019 04:05
@schmidt-sebastian schmidt-sebastian merged commit 218a4c6 into master Jan 2, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/gaxretries branch January 3, 2020 00:33
@mad-it mad-it mentioned this pull request Jan 7, 2020
Closed
@yoshi-automation yoshi-automation mentioned this pull request Jan 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-fenster alexander-fenster alexander-fenster approved these changes

Assignees

@alexander-fenster alexander-fenster

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schmidt-sebastian @alexander-fenster @googlebot