-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
rafthttp: configurable stream reader retry timeout #8003
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,8 +94,10 @@ type Transporter interface { | |
// User needs to call Start before calling other functions, and call | ||
// Stop when the Transport is no longer used. | ||
type Transport struct { | ||
DialTimeout time.Duration // maximum duration before timing out dial of the request | ||
TLSInfo transport.TLSInfo // TLS information used when creating connection | ||
DialTimeout time.Duration // maximum duration before timing out dial of the request | ||
DialRetryTimeout time.Duration // alters the frequency of streamReader dial retrial attempts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change this to DialRetryLimiter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xiang90 do you mean to have a single instance of If this is a desired behavior, we definitely can put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh. ok. i see. i agree probably better to limit per stream (we need to document this better though). my main motivation is to change timeout to something like rate. retry timeout is usually used to describe the timeout of the entire retry (when you give up on retries). we, here, actually retry forever and there is a backoff between each retry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point... May be variable of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vitalyisaev2 right. can you give it a try? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xiang90 Yes, sure |
||
|
||
TLSInfo transport.TLSInfo // TLS information used when creating connection | ||
|
||
ID types.ID // local member ID | ||
URLs types.URLs // local peer URLs | ||
|
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.
not a fan of this forced timeout, would it be possible to replace
stopc
withand replace the
select
with:and replace any
<-stopc
with<-runCtx.Done()
?
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.
@heyitsanthony that's right, I've replaced
streamReader.stopc
withstreamReader.ctx
andstreamReader.cancel
.