Only fallback to leader read with data-is-not-ready error #907
Conversation
|
@you06 |
|
/cc @crazycs520 PTAL |
|
@you06 Please resolve the conflicts. |
internal/locate/region_request.go
Outdated
| @@ -604,7 +605,7 @@ func (state *accessFollower) isCandidate(idx AccessIndex, replica *replica) bool | |||
| // The request can only be sent to the leader. | |||
| ((state.option.leaderOnly && idx == state.leaderIdx) || | |||
| // Choose a replica with matched labels. | |||
| (!state.option.leaderOnly && (state.tryLeader || idx != state.leaderIdx) && replica.store.IsLabelsMatch(state.option.labels))) && | |||
| (!state.option.leaderOnly && (state.tryLeader || idx != state.leaderIdx) && (state.lastIdx >= 0 || replica.store.IsLabelsMatch(state.option.labels)))) && | |||
| // Make sure the replica is not unreachable. | |||
| replica.store.getLivenessState() != unreachable | |||
There was a problem hiding this comment.
It's better to split this check into different checks like the master branch here like:
func (state *accessFollower) isCandidate(idx AccessIndex, replica *replica) bool {
if condition1 not match
return false
if condition2 not match
return false
if condition3 matches
return true
return false
}
, it's quite difficult to maintain.
There was a problem hiding this comment.
Done, and I did this for master branch before.
internal/locate/region_request.go
Outdated
| @@ -604,7 +605,7 @@ func (state *accessFollower) isCandidate(idx AccessIndex, replica *replica) bool | |||
| // The request can only be sent to the leader. | |||
| ((state.option.leaderOnly && idx == state.leaderIdx) || | |||
| // Choose a replica with matched labels. | |||
| (!state.option.leaderOnly && (state.tryLeader || idx != state.leaderIdx) && replica.store.IsLabelsMatch(state.option.labels))) && | |||
| (!state.option.leaderOnly && (state.tryLeader || idx != state.leaderIdx) && (state.lastIdx >= 0 || replica.store.IsLabelsMatch(state.option.labels)))) && | |||
There was a problem hiding this comment.
Why adding (state.lastIdx >= 0 || here?
There was a problem hiding this comment.
I change this condition to leaderExhaust.
The try order is local follower -> leader -> remote follower.
When leaderExhaust is true, we need to ignore the labels to select the remote follower.
There was a problem hiding this comment.
Is is working as expected when the local follower is the leader peer picked first and it's unavailable for both stale and follower read requests?
It's better to comment on it in detail here.
Signed-off-by: you06 <[email protected]> typo Signed-off-by: you06 <[email protected]> add test Signed-off-by: you06 <[email protected]> remove comment Signed-off-by: you06 <[email protected]> remove comment Signed-off-by: you06 <[email protected]> update PR Signed-off-by: you06 <[email protected]> Update internal/locate/region_request.go Co-authored-by: cfzjywxk <[email protected]>
Signed-off-by: you06 <[email protected]>
you06
force-pushed
the
reset-stale-read-6.5
branch
from
a408188 to
31ad539
Compare
Signed-off-by: you06 <[email protected]>
| for i := 0; i < len(selector.replicas) && !state.option.leaderOnly; i++ { | ||
| idx := AccessIndex((int(state.lastIdx) + i) % len(selector.replicas)) | ||
| selectReplica := selector.replicas[idx] | ||
| if state.isCandidate(idx, selectReplica) { | ||
| if state.isCandidate(idx, selectReplica, leaderExhaust) { |
There was a problem hiding this comment.
The leaderExhaust which is the result of state.IsLeaderExhausted should not be used here in the isCandidate function as the leaderExhaust checks isExhausted with 2 and the isCandidate checks only 1. The leaderExhaust is designed to be only used in the L574 if selector.targetIdx < 0 { branch to allow an extra retry on the leader peer if no peers are available.
There was a problem hiding this comment.
I'm still trying to figure out the results of the different combinations here and if they all work as expected...
The conditions of combinations include:
- A stale read or a follower read.
- The local peer matching label is the leader peer or not.
- No fallback happens.
- After falling back to leader read, if the leader is still unavailable, if the error would be returned to upper layer as expected.
Quite difficult to maintain by now...
|
@you06 Is this to be closed? |
Inject server-is-busy error for stale read and leader read, so direct fallback to leader won't success.
This patch will continue trying other replicas(the duration is high because of backoff).