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
F2205338: D10329.diff
Sat, Jul 6, 8:35 PM
Unknown Object (File)
Tue, Jul 2, 3:25 AM
Unknown Object (File)
Sun, Jun 30, 2:05 PM
Unknown Object (File)
Fri, Jun 28, 12:24 AM
Unknown Object (File)
Mon, Jun 24, 1:55 PM
Unknown Object (File)
Sun, Jun 23, 11:26 PM
Unknown Object (File)
Sun, Jun 23, 11:26 PM
Unknown Object (File)
Sun, Jun 23, 11:26 PM
View All 35 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.

Revision Contents
Changeset List

PathSize
shared/
grpc_clients/
src/
5 lines

Diff 34686

View Options

shared/grpc_clients/src/lib.rs

Loading...