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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement settable timeouts in Functions. #224

Merged
merged 6 commits into from Feb 25, 2019

Conversation

Projects
None yet
5 participants
@bklimt
Copy link
Contributor

commented Jan 30, 2019

  1. Update okhttp to a newer version.
  2. Change the default timeout for Functions to 60 seconds, to match the backend.
  3. Add methods to change the timeout for a Function.
  4. Ensure all exceptions coming from Functions are wrapped in FirebaseFunctionsException.

bklimt added some commits Jan 30, 2019

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

/retest

1 similar comment
@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

/retest

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

These oss-bot failures don't look like they have anything to do with my changes.

bklimt added some commits Feb 1, 2019

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

/retest

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

This was implemented with okhttp 3, because the newer version: 1) lets us specify a single timeout for the whole call, instead of separate timeouts for connect, read, and write, and 2) lets us easily set timeouts for particular calls, rather than having to create a new client for each callable.

https://github.com/square/okhttp/wiki/Recipes#timeouts

The downside is this may increase library size for developers using both Firestore and Functions, since they will no longer share the same okhttp version.

@bklimt bklimt requested a review from ashwinraghav Feb 4, 2019

@CodingDoug

This comment has been minimized.

Copy link

commented Feb 5, 2019

Is 60s the best number on the client? If that's measured by the moment the client attempts to connect, to the moment it receives the response, the function on the server may not actually have its own full 60s to yield a response, since the client could also be waiting for network latency and such.

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

60s is what had been agreed upon for the API, and it's a big improvement over the previous, unspecified default. But I agree it would be reasonable to have a longer default to account for connection time. We should make the default the same across all client SDKs. What length default timeout would you propose?

@CodingDoug

This comment has been minimized.

Copy link

commented Feb 6, 2019

I'd guess that a buffer of 5 extra seconds is enough to cover 98% of the connections out there.

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

After some discussion offline, I've upped the default timeout to 70s. Thanks for the feedback @CodingDoug.

@bklimt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

/retest

@google-oss-bot

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

@bklimt: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
smoke-tests-release 4496521 link /test smoke-tests-release

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Show resolved Hide resolved firebase-functions/firebase-functions.gradle
Show resolved Hide resolved ...ctions/src/main/java/com/google/firebase/functions/HttpsCallOptions.java
* @param timeout The length of the timeout, in the given units.
* @param units The units for the specified timeout.
*/
public void setTimeout(long timeout, TimeUnit units) {

This comment has been minimized.

Copy link
@ashwinraghav

ashwinraghav Feb 14, 2019

Collaborator

Does this require an API review?

This comment has been minimized.

Copy link
@bklimt

bklimt Feb 14, 2019

Author Contributor

Yes, the API has been approved. go/callable-timeouts

Show resolved Hide resolved ...tions/src/main/java/com/google/firebase/functions/FirebaseFunctions.java
@ashwinraghav
Copy link
Collaborator

left a comment

Thanks for your patience

@bklimt bklimt merged commit 99d6465 into master Feb 25, 2019

7 of 8 checks passed

smoke-tests-release Job failed.
Details
build-plugins-check Skipped
check-changed Job succeeded.
Details
cla/google All necessary CLAs are signed
connected-check-changed Job succeeded.
Details
copyright-check Job succeeded.
Details
fireci Skipped
smoke-tests-debug Job succeeded.
Details

@vkryachko vkryachko deleted the klimt.functions-timeouts branch Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.