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.

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

Already on GitHub? Sign in to your account

feat: Use REST #1698

Merged
merged 8 commits into from Sep 12, 2022
Merged

feat: Use REST #1698

merged 8 commits into from Sep 12, 2022

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Mar 14, 2022

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 馃

@schmidt-sebastian schmidt-sebastian requested review from a team as code owners March 14, 2022 21:23
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Mar 14, 2022
@bcoe
Copy link
Contributor

bcoe commented Mar 16, 2022

@schmidt-sebastian this is cool, is your intention to default to gRPC, and document REST?

@schmidt-sebastian
Copy link
Contributor Author

@bcoe I haven't decided yet. We need to fix one more issue with queries and then compare performance data to see what mode should be the default.

@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Apr 8, 2022
@MarkDuckworth MarkDuckworth added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 23, 2022
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 24, 2022
@MarkDuckworth MarkDuckworth added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels May 24, 2022
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 24, 2022
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Jul 25, 2022
@MarkDuckworth MarkDuckworth added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 25, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 25, 2022
@MarkDuckworth MarkDuckworth added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 25, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 25, 2022
@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 30, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 30, 2022
@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2022
@alexander-fenster
Copy link
Member

alexander-fenster commented Aug 31, 2022

@MarkDuckworth It works now! When I throw new Error('...') in node_modules/@grpc/grpc-js/build/src/index.js and run npm run system-test:rest, only a handful of tests that actually require gRPC fail, most tests pass.

Note: I'm using the pre-released version of google-gax because of the bug. I will remove the package.json change when gax is released later today.

The main change is in the client factory here: https://github.com/googleapis/nodejs-firestore/pull/1698/files#diff-33330e7a0daf2387ffc6f936c4d6cb4dd23b4b404446a5e34d2cab779e47cb50R552

@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

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

Looks great! One minor suggestion but I don't feel very strongly about it.

status.message || undefined
);
const error =
new (require('google-gax/build/src/fallback').GoogleError)(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if instead of using this path to fallback, we can use a subpath export to the fallback? https://nodejs.org/api/packages.html#subpath-exports

Maybe it's already set up that way, I didn't look at gax source yet. But the path indicates maybe it is not.

Copy link
Member

Choose a reason for hiding this comment

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

That's a great idea (and "today I learned" as well).

We will be able to safely start using it when we drop Node 12 support. The documentation says subpath exports were added in v12.7.0 but we have "engines.node" set to ">=12" now, so we cannot possibly start using it without making a semver major. But for the next major I definitely want to do it. I filed googleapis/gax-nodejs#1337 so that we don't forget.

@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 2, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 2, 2022
@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2022
@alexander-fenster alexander-fenster merged commit d85b0e9 into main Sep 12, 2022
@alexander-fenster alexander-fenster deleted the mrschmidt/restclient branch September 12, 2022 20:49
@hspak
Copy link

hspak commented Feb 2, 2023

Hello! Sorry for reviving an old pull request, but was this data published anywhere?

We need to fix one more issue with queries and then compare performance data to see what mode should be the default.

@MarkDuckworth
Copy link
Contributor

Performance data was not published. From what I have seen, internal tests and customer reports indicate a speedup when using preferRest in the cold start of functions. However, I cannot give specific numbers.

We did not change the default mode, which is still gRPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants