-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
server,rpc,circuit: assortment of small cleanups/improvements #103581
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.
LGTM but I thought you wanted to use the prev
error somewhere?
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 3 of 3 files at r8, 2 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @tbg)
pkg/util/circuit/circuitbreaker.go
line 153 at r5 (raw file):
// Outside of testing, there should be no reason to call this // as it is the probe's job to reset the breaker if appropriate. func (b *Breaker) Reset() {
Slightly easier on the eye:
var prev error
defer func() { b.Opts().EventHandler.OnReset(b, prev) }()
b.mu.Lock()
defer b.mu.Unlock()
if wasTripped := b.mu.errAndCh.err != nil; wasTripped {
prev = b.mu.errAndCh.err
b.mu.errAndCh = b.newErrAndCh()
}
pkg/util/circuit/event_handler.go
line 66 at r5 (raw file):
func (d *EventLogger) OnReset(breaker *Breaker, prev error) { var buf redact.StringBuilder EventFormatter{}.OnReset(breaker, &buf)
Not using prev
here?
ab376a0
to
998b87c
Compare
TFTR! I decided to drop the commit you were working on. I originally anticipated using a custom event handler in #99191 but this didn't materialize, so the new code would be unused. (I don't think it's useful for the default impl of EventHandler to log the previous error when the breaker resets). |
Can't scan from `/Min`. Epic: none Release note: None
What it said used to be true, but it caused all kinds of problems and so we've stopped sharing long ago. Epic: None Release note: None
It will be convenient to refer to it instead of typing out the anonymous interface multiple times. I'm not sure why I didn't introduce this much earlier, must have been a long bout of misguided purism. Epic: none Release note: None
A type peer will be introduced soon. Epic: None Release note: None
Epic: none Release note: None
Epic: none Release note: None
Epic: none Release note: None
I was really confused for a while by what I thought was odd RPC connection behavior. Knowing as I do now that the test restarts a store (from within a helper function) explains everything. Make this clearer via a rename. Epic: none Release note: None
bors r=knz |
Build succeeded: |
Extracted from #99191.
backoff
packageRelease note: None
Epic: None