RFC: etcdserver, pkg: skip needless log entry applying #5689
Conversation
|
this is useful especially now linearizable read still needs to go through raft apply path. We are optimizing the l-read though. After we make l-read only applied by one local member, we probably want to optimize the txn apply, which might also contain a large range query. That requires more fine fine grained checking though. But I think that is probably the right direction to move this forward. |
|
@xiang90 thanks for your comments. Yes, the l-read processed by follower would gain benefit from this change like serializable read (cases that don't involve local l-read and serializable read wouldn't be so interested in this change because skipping applying will increase free time of followers but they can't do anything in the free time). I'll refine this change. |
|
@xiang90 updated for the test failure. And now serializable txn is treated as no side effect request. |
|
updated for treating read only txn as no side effect request (previous version treated serializable txn as no side effect request, but serializable txn shouldn't be processed in apply phase) |
|
@xiang90 do you need this PR (e.g. after v3.0.0 release)? Then I can rebase it on the latest master. |
|
yes. i have more thought on this. i will think more this week. |
|
@xiang90 rebased, PTAL when you are available. |
|
|
||
| func noSideEffect(r *pb.InternalRaftRequest) bool { | ||
| if r.Txn != nil { | ||
| return isTxnReadonly(r.Txn) |
There was a problem hiding this comment.
actually we do not even need to apply the range inside txns.
There was a problem hiding this comment.
probably we can do the txn one later inside the actual txn apply func. we only count range, authget as noSideEffect here.
There was a problem hiding this comment.
Ah, do you mean isTxnReadonly() should check the condition like below?
GetRequestPut() == nil && GetRequestDeleteRange() == nil?
There was a problem hiding this comment.
ok I'll remove the checking for txn for now
There was a problem hiding this comment.
No. When we start to apply the txn here:
https://github.com/coreos/etcd/blob/master/etcdserver/apply.go#L366
https://github.com/coreos/etcd/blob/master/etcdserver/apply.go#L453
We can actually skip range when there is no waiter. So even if txn is not readonly, we can still skip applying the readonly part of it.
There was a problem hiding this comment.
Ah I see. BTW, will the txn feature not support putting value that depends on a result of other range reqs in the same transaction? e.g. x = range(); put(modify(x)) in single txn
If there is no such a plan (I'm not sure it is useful or not), your plan seems to be good.
There was a problem hiding this comment.
there is no plan for that (yet)...
There was a problem hiding this comment.
ok then I'll follow your direction
This commit lets etcdserver skip needless log entry applying. If the result of log applying isn't required by the node (client that issued the request isn't talking with the node) and the operation has no side effects, applying can be skipped. It would contribute to reduce disk I/O on followers and be useful for a cluster that processes much serializable get.
|
@xiang90 updated for your comments, PTAL. txn part isn't cared for now. |
|
lgtm |
|
@xiang90 can I merge this? |
|
Yes |
|
@xiang90 thanks, merged. |
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
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
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
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
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
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
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
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
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
CAUTION: This is just an experimental PR.
This commit lets etcdserver skip needless log entry applying. If the
result of log applying isn't required by the node (client that issued
the request isn't talking with the node) and the operation has no side
effects, applying can be skipped.
I'm completely not sure this is useful for practical cases (it would
be useful for a cluster that processes much serializable get). I tried
the idea because I saw many too long apply entry warning on my
development environment.