etcdserver: skip range requests in txn if the result is needless #5911
Conversation
|
kindly ping? @xiang90 |
| @@ -49,12 +49,12 @@ type applyResult struct { | |||
|
|
|||
| // applierV3 is the interface for processing V3 raft messages | |||
| type applierV3 interface { | |||
| Apply(r *pb.InternalRaftRequest) *applyResult | |||
| Apply(r *pb.InternalRaftRequest, hasWaiter bool) *applyResult | |||
There was a problem hiding this comment.
hasWaiter -> canSkip? From applier perspective, haswaiter does not make sense.
There was a problem hiding this comment.
canSkip seems to be better. I'll rename the parameter, thanks.
|
lgtm. need to rebase. sorry for the review delay. |
|
@xiang90 updated for the renaming and rebasing , PTAL. |
| ar = s.applyV3.Apply(&raftReq) | ||
| canSkip := !s.w.IsRegistered(id) | ||
| if !canSkip || !noSideEffect(&raftReq) { | ||
| ar = s.applyV3.Apply(&raftReq, false) |
There was a problem hiding this comment.
s.applyV3.Apply(&raftReq, canSkip)?
There was a problem hiding this comment.
Ah, thanks for pointing, I'll fix it.
|
@xiang90 updated for your comment, PTAL. |
|
@mitake This interface change seems somewhat excessive for what it does. I think it would be cleaner to strip out the range requests from the txn request before passing it to Apply instead of having a special flag for this edge case. What do you think? |
|
@heyitsanthony yes, your idea seems to be far better than the current implementation. I'll fix this PR later. |
|
@heyitsanthony @xiang90 updated for simplifying, PTAL. |
| for i := 0; i < len(ops); { | ||
| switch ops[i].Request.(type) { | ||
| case *pb.RequestOp_RequestRange: | ||
| ops = append(ops[:i], ops[i+1:]...) |
There was a problem hiding this comment.
probably just nil it out instead of appending? then we can avoid some allocations...
|
@xiang90 updated for your comment, PTAL. |
|
lgtm defer to @heyitsanthony |
|
lgtm |
|
|
||
| func removeNeedlessRangeReqs(txn *pb.TxnRequest) { | ||
| f := func(ops []*pb.RequestOp) { | ||
| for i := 0; i < len(ops); i++ { |
There was a problem hiding this comment.
if you want to avoid all the nil checks + keep the alloc-free stuff,
j := 0
for i := 0; i < len(ops); i++ {
if _, ok := ops[i].Request.(*pb.RequestOp_RequestRange); ok {
continue
}
ops[j] = ops[i]
j++
}
return ops[:j]
...
txn.Success = f(txn.Success)
txn.Failure = f(txn.Failure)
There was a problem hiding this comment.
thanks, it seems to be better. I'll follow this style and remove the nil checks in the apply phase.
|
@heyitsanthony @xiang90 updated for the better removal of range reqs, PTAL |
If a server isn't serving txn requests from a client, the server doesn't need the result of range requests in the txn. This is a succeeding commit of etcd-io#5689
|
lgtm, thanks! |
If a server isn't serving txn requests from a client, the server
doesn't need the result of range requests in the txn.
This is a succeeding commit of
#5689
/cc @xiang90