-
Notifications
You must be signed in to change notification settings - Fork 16
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
set deadlines on outbound requests #2485
Conversation
Generate changelog in
|
d32f0ab
to
2a2d58f
Compare
@@ -242,6 +242,8 @@ private static ImmutableList<LimitedChannel> createHostChannels( | |||
channel = HostMetricsChannel.create(cf, channel, targetUri.uri()); | |||
channel = | |||
new TraceEnrichingChannel(channel, DialogueTracing.tracingTags(cf, uriIndexForInstrumentation)); | |||
channel = new DeadlineAdvertisementChannel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there other places we need to create one of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be -- this should do the trick! This position looks great because executes after the queues and retrying channels so that it will be re-applied on each request with an updated Expect-With
value
@@ -242,6 +242,8 @@ private static ImmutableList<LimitedChannel> createHostChannels( | |||
channel = HostMetricsChannel.create(cf, channel, targetUri.uri()); | |||
channel = | |||
new TraceEnrichingChannel(channel, DialogueTracing.tracingTags(cf, uriIndexForInstrumentation)); | |||
channel = new DeadlineAdvertisementChannel( | |||
channel, cf.clientConf().readTimeout()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple places which set a read timeout value of zero expecting it to be infinite. We handle this case in the apache httpclient shim here:
Lines 641 to 648 in f4915fc
if (socketTimeoutMillis == 0) { | |
// https://issues.apache.org/jira/browse/HTTPCLIENT-2099 | |
log.debug( | |
"Working around HTTPCLIENT-2099 by using a 1 day socket " | |
+ "timeout instead of zero (unlimited). Client: {}", | |
SafeArg.of("client", clientName)); | |
socketTimeoutMillis = Duration.ofDays(1).toMillis(); | |
} |
I think we'll need to handle that case in the DeadlineAdvertisementChannel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 fixed!
Released 5.1.0 |
Before this PR
No
Expect-Within
header is added to outbound requestsAfter this PR
Adds the deadlines-java library and utilizes it to set
Expect-Within
headers on outbound requests.This change adds a
DeadlineAdvertisementChannel
which callsDeadlines.encodeToRequest
to set anExpect-Within
header equal to the configured socket read timeout for the client, or a remaining deadline read from aTraceLocal
per the implementation in deadlines-java (whichever is smaller).deadlines-java does not currently doe any enforcement on deadline expiration, and instead only reports metrics when a deadline is exceeded before sending an outbound request.
==COMMIT_MSG==
set deadlines on outbound requests
==COMMIT_MSG==
Possible downsides?