-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
caboose.go
Outdated
@@ -85,11 +85,12 @@ const DefaultLoggingInterval = 5 * time.Second | |||
const DefaultSaturnOrchestratorRequestTimeout = 30 * time.Second | |||
|
|||
const DefaultSaturnBlockRequestTimeout = 19 * time.Second | |||
const DefaultSaturnCarRequestTimeout = 30 * time.Minute | |||
|
|||
const DefaultSaturnCarRequestTimeout = 1 * time.Minute |
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.
Max P95 for a successful CAR fetch over the last 24 hours in prod is 30 seconds:
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.
does this mean that a large movie won't work anymore?
I don't think this is an acceptable limit.
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.
Hmmm, I see what you mean.
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.
@willscott Reverted this change.
const DefaultSaturnMirrorRequestTimeout = 30 * time.Second | ||
|
||
// default retries before failure unless overridden by MaxRetrievalAttempts | ||
const defaultMaxRetries = 3 | ||
const defaultMaxRetries = 2 |
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.
~99% of ALL requests that succeed do so in the first two retries:
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.
this seems fine
@@ -236,9 +236,9 @@ func (p *pool) fetchResource(ctx context.Context, from string, resource string, | |||
// which is the amount of time without any NEW data from the server, but | |||
// that can be added later. We need both because a slow trickle of data | |||
// could take a large amount of time. | |||
requestTimeout := DefaultSaturnCarRequestTimeout | |||
requestTimeout := time.Duration(DefaultSaturnCarRequestTimeout.Milliseconds()/int64(attempt+1)) * time.Millisecond |
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.
Reduces timeout duration with each retry.
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.
this seems okay
Improve retry logic