raftpicker: do not use picker #1313
Conversation
Current coverage is 55.12% (diff: 25.00%)@@ master #1313 diff @@
==========================================
Files 80 80
Lines 12562 12583 +21
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 6933 6936 +3
- Misses 4674 4690 +16
- Partials 955 957 +2
|
|
This is one more fix for problem with wrong rescheduling on leader losing. |
aaronlehmann
added
status/2-code-review
process/cherry-pick
and removed
status/0-triage
labels
| } | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
To reduce code duplication, could we put this part in a const?
LK4D4
force-pushed
the
no_raft_picker
branch
2 times, most recently
from
a42a3e8
to
3a69ab3
Compare
|
@aaronlehmann fixed. Thanks! |
|
LGTM if this works well in practice. |
| func (p *picker) PickAddr() (string, error) { | ||
| addr, err := p.raft.LeaderAddr() | ||
| // Reset recreates underlying connection. | ||
| func (c *ConnSelector) Reset() { |
There was a problem hiding this comment.
Any reason to remove the return error from Reset()? I feel like having return error would show if the connection is good or bad.
There was a problem hiding this comment.
It's called only in raftproxy and I was reluctant to add logrus import there. Do you think I should just add return and left logging here as well?
There was a problem hiding this comment.
Yes I think having return error is better. Logging can be done where it's convenient.
LK4D4
force-pushed
the
no_raft_picker
branch
2 times, most recently
from
5a136c8
to
1285516
Compare
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| if c.cc != nil { | ||
| c.cc.Close() |
There was a problem hiding this comment.
Is double Close() allowed or not? I think there may be paths leading to double close. If it's a problem, it may make sense to set c.cc = nil right after c.cc.Close().
There was a problem hiding this comment.
It's not a problem, but I'll add this guard just for the case.
Just recreate connection on leader change. Signed-off-by: Alexander Morozov <lk4d4math@gmail.com>
|
LGTM |
|
I'm fine with the change but on leader switch now I get: And I have to input the command a second time to have the result. Is this the behavior we want? |
|
@abronan I think it's lesser evil. ping @aaronlehmann |
|
Yeah. |
|
LGTM |
Just recreate connection on leader change.
ping @aaronlehmann