Page MenuHomePhabricator
Differential D10329

[grpc_clients] add connect_timeout to shared grpc client
ClosedPublic
Actions

Authored by varun on Dec 13 2023, 6:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Sep 14, 5:09 PM
Unknown Object (File)
Sat, Sep 7, 9:27 AM
Unknown Object (File)
Sat, Sep 7, 9:27 AM
Unknown Object (File)
Sat, Sep 7, 9:27 AM
Unknown Object (File)
Fri, Sep 6, 10:29 PM
Unknown Object (File)
Sep 4 2024, 10:18 AM
Unknown Object (File)
Sep 4 2024, 7:42 AM
Unknown Object (File)
Sep 4 2024, 6:30 AM
View All 49 Files
Subscribers
tomek

Details

Reviewers
ashoat
bartek
Commits
rCOMMd4f399fb614e: [grpc_clients] add connect_timeout to shared grpc client
Summary

by default, the shared grpc client will try to connect indefinitely to the endpoint url. we should time out after 1 second if we're unable to connect.

Test Plan

repro'd ENG-6052 and then added the time out and confirmed that the client only waited 1 second before giving up

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun created this revision.Dec 13 2023, 6:54 PM
Herald added a subscriber: tomek. · View Herald TranscriptDec 13 2023, 6:54 PM
varun added a comment.Dec 13 2023, 6:56 PM

this fixed the same indefinite connection attempt i saw on native too

Harbormaster completed remote builds in B25057: Diff 34617.Dec 13 2023, 7:13 PM
varun requested review of this revision.Dec 13 2023, 7:13 PM
bartek requested changes to this revision.Dec 14 2023, 12:03 AM
bartek added inline comments.
shared/grpc_clients/src/lib.rs
21 ↗(On Diff #34617)

Isn't this timeout way too short? Maybe for localhost testing it's enough, but real networking isn't that stable.
Let's give it at least 5 seconds or so (which is still short IMO)

This revision now requires changes to proceed.Dec 14 2023, 12:03 AM
varun updated this revision to Diff 34684.Dec 14 2023, 6:57 PM

increase timeout duration

shared/grpc_clients/src/lib.rs
21 ↗(On Diff #34617)

1 second was enough for staging, too, but i think it's fine to bump it to 5

Harbormaster completed remote builds in B25099: Diff 34684.Dec 14 2023, 7:17 PM
bartek accepted this revision.Dec 14 2023, 10:32 PM
This revision is now accepted and ready to land.Dec 14 2023, 10:32 PM
Closed by commit rCOMMd4f399fb614e: [grpc_clients] add connect_timeout to shared grpc client (authored by varun). · Explain WhyDec 14 2023, 10:38 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMMd4f399fb614e: [grpc_clients] add connect_timeout to shared grpc client.
varun mentioned this in D13355: [grpc_clients] bump gRPC connection timeout to 30 seconds.Mon, Sep 16, 3:57 PM
varun mentioned this in rCOMM849adf8a969d: [grpc_clients] bump gRPC connection timeout to 30 seconds.Tue, Sep 17, 8:59 AM

Revision Contents
Changeset List

PathSize
shared/
grpc_clients/
src/
5 lines

Diff 34686

View Options

shared/grpc_clients/src/lib.rs

Loading...