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

	F3519708: D10329.id34686.diff
	Sun, Dec 22, 10:45 PM

	F3519707: D10329.id34684.diff
	Sun, Dec 22, 10:45 PM

	F3519706: D10329.id34617.diff
	Sun, Dec 22, 10:45 PM

	F3519696: D10329.id.diff
	Sun, Dec 22, 10:45 PM

	F3519603: D10329.diff
	Sun, Dec 22, 10:43 PM

	Unknown Object (File)
	Fri, Nov 29, 12:18 AM

	Unknown Object (File)
	Mon, Nov 25, 6:14 PM

	Unknown Object (File)
	Mon, Nov 25, 5:09 PM

View All 68 Files

Subscribers

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

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

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

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.Sep 16 2024, 3:57 PM

varun mentioned this in rCOMM849adf8a969d: [grpc_clients] bump gRPC connection timeout to 30 seconds.Sep 17 2024, 8:59 AM

Revision Contents
Changeset List

Path

Size

shared/

grpc_clients/

src/

5 lines

Diff 34686

shared/grpc_clients/src/lib.rs

Loading...