-
Notifications
You must be signed in to change notification settings - Fork 125
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
conncache - skip caching errors #5418
base: main
Are you sure you want to change the base?
Conversation
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.
Seems like this just skips caching errors entirely? Having a TTL as described in the issue might be preferable: #5076
Also, there are CI failures.
I guess, you propose something like this: go func() {
for true {
time.Sleep(5 * time.Second)
go func() {
c.mu.Lock()
defer c.mu.Unlock()
for k, v := range c.entries {
if v.err != nil && time.Now().Sub(v.previousErrTime) > 100*time.Second {
c.beginClose(k, v)
}
}
}()
}
}() |
CI fixed, need a design review. |
runtime/pkg/conncache/conncache.go
Outdated
// The maximum time the error stays in cache | ||
ErrorTimeout time.Duration |
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.
- It looks like this is not set anywhere?
- Consider calling it
ErrorTTL
instead since it's not a timeout the same way thatOpenTimeout
andCloseTimeout
are
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.
Done.
runtime/pkg/conncache/conncache.go
Outdated
go func() { | ||
for { | ||
time.Sleep(5 * time.Second) | ||
go func() { |
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 is missing cancellation from Close
. Instead of having a background loop (which adds concurrency complexity), how about just checking the TTL when accessing a cached error in Acquire
? That should still ensure correctness, and cleanup for connections that are not acquired again anyway happens automatically by the LRU cache
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.
Done.
Tests passed. |
Depends on this #5218