Read index retry #12780
Read index retry #12780
Conversation
| lg := s.Logger() | ||
| errorTimer := time.NewTimer(s.Cfg.ReqTimeout()) |
There was a problem hiding this comment.
I don't understand it...
Would following approach work:
- retryTimer instead of errorTimer.
- both selects merged into a single select.
?
There was a problem hiding this comment.
First, I've decided to use 2 selects instead of one because of this article. In short: it is to guard against picking case at random when multiple channels are unblocked. It seemed reasonable when writing code, however now I can't say any good reason to have it this way, so I'll move it to single select.
Second think is "retryTimer instead of errorTimer". I'm not sure if I understand you correctly, but if you suggest having retryTimer (one that measure 500ms) outside of for loop and putting errorTimer (one that measure 7s) in <-time.After(...), then it would not work as we need to have single errorTimer across potentially multiple retryTimer.
There was a problem hiding this comment.
A.d. 1. I see. But I think we don't really care which branch will be taken if it happens that multiple activate exactly at the same time. So would go for merging.
A.d. 2: You are right. Potentially you can use top-level 'ticker' (time.NewTicker) to get periodic notification to refresh the request. The benefit is that it automatically will cancel goroutine.
There was a problem hiding this comment.
A.d. 2:
I see Timer more intuitive than Ticker here, as "ping me after 500ms" describes concept of timeout better than "ping me every 500ms". However I don't understand below part
The benefit is that it automatically will cancel goroutine
Could you elaborate?
There was a problem hiding this comment.
<-time.After(readIndexRetryTime)
under the cover starts go-routine the sleep's some time and populates the channel.
If the select exits for another reason, the goroutne still exists for 'up to 500ms' to populate the channel (that no-one is waiting for).
There was a problem hiding this comment.
Good point. I've changed it to single timer initialised outside of loop and I use retryTimer.Reset(readIndexRetryTime) to refresh it.
wpedrak
force-pushed
the
read_index_retry
branch
from
6d492e4
to
1df6caf
Compare
It is second approach (with first being etcd-io#12762) to solve etcd-io#12680
wpedrak
force-pushed
the
read_index_retry
branch
from
1df6caf
to
e977923
Compare
@Cjen1 evaluated the solution in #12680 (comment). Bottom 3 charts show reduction of the delay to ~1s with etcd-retry solution. etcd-postpone solution was in 2 cases ~700ms and in one ~1300ms. |
It is second approach (with first being #12762) to solve #12680
This PR is composed of 2 commit, first being refactor of l-read loop and second being implementation of retry mechanism itself.
Drawbacks of this change (would like to seek your opinion on it):