*: rewrite snapshot merging and sending for v3#3970
Merged
xiang90 merged 1 commit intoetcd-io:masterfrom Dec 10, 2015
Merged
Conversation
Contributor
There was a problem hiding this comment.
where do we get the size of buffer 128? If arbitrary, we should briefly comment.
Contributor
|
New code looks cleaner. Look forward to seeing more updates. Left some comments. |
Contributor
Author
|
@gyuho I cleaned up a bunch of things. Now it is reviewable. |
Contributor
|
Thanks, I will have a look |
Contributor
There was a problem hiding this comment.
Does this mean that it forwards to leader?
Contributor
Author
There was a problem hiding this comment.
this (change To to 0) means the message will be dropped. see line 823 too. probably we should doc it.
we drop the original snapshot message since etcdserver will send it later when it gets merged snapshot.
Contributor
There was a problem hiding this comment.
Oh got it. Yeah I couldn't find documentation on that.
Contributor
Author
There was a problem hiding this comment.
It is doced somewhere in rafthttp. :(
Contributor
|
@xiang90 I read all the change and makes sense to me, but it's a big change. Do you plan any more updates on this PR? Otherwise, I will run the actual cluster with this code tomorrow to double check. |
Contributor
Author
It will fail since there is an issue with bolt db. I will update you with details tomorrow. |
Contributor
Author
No. I do not. |
Contributor
|
Got it. What was the issue with boltdb? Code-wise, LGTM. |
Contributor
Author
|
@gyuho I will update with you more details about boltdb tomorrow or the day after tomorrow. |
Contributor
|
Got it. Thanks! I will keep reviewing this to understand more. |
Contributor
Author
|
@gyuho The issue with boltdb is that its reader might block writer. So when sending out a large snapshot (open a long running read txn in bolt), the writes might get blocked. This is what we do not want to see. We have a pending pr at boltdb/bolt#432 |
Contributor
Author
|
Other than that feel free to test it out. What I have done is to set a 3 nodes cluster with v3 enabled. Then I use benchmark tool to generate enough puts to trigger snapshot while killing nodes to force snapshot sending. |
Contributor
|
Cool, I will test it out as well. |
Contributor
|
I tested this with benchmark tool and it works well. LGTM. |
Contributor
Author
|
@gyuho Did you trigger a snapshot sending? |
Contributor
|
Yes |
Contributor
Author
|
@gyuho Great. I will keep this open until tomorrow night. You can try to play with it more. |
Contributor
|
Sure, thanks, On Wed, Dec 9, 2015 at 3:15 PM, Xiang Li notifications@github.com wrote:
Sincerely, |
Contributor
Author
|
BTW This is not a snapshot sending. This is just etcdserver doing a snapshot. You have to kill a etcd member, force etcd leader to compact its log. Then when the killed member comes up, the leader does not have required log to send to it (due to compaction). So the leader will have to send a snapshot to that member. You will see log like recovering from incoming snapshot at the other side. |
Contributor
|
@xiang90 +1. Thanks! |
Contributor
|
@xiang90 I tested that scenario.
And I checked that it generates log compaction. Please go over this log. |
Contributor
|
And ran this with this command: |
Contributor
Author
|
@gyuho Yea. That is what I expected. The log is still a little bit noisy. We need to improve it over time. |
xiang90
added a commit
that referenced
this pull request
Dec 10, 2015
*: rewrite snapshot merging and sending for v3
Contributor
|
@xiang90 Yeah other than that, code works great. Thanks, |
Contributor
|
1million_first.txt @xiang90 So I tried to see blocking behavior from boltdb. First case is with 1 million keys, without boltdb/bolt#432. Second case is with 1 million keys and with boltdb/bolt#432. Do you have any suggestion to benchmark the Please let me know. |
Contributor
Author
|
@gyuho There should not be any perf difference. You should try to create a case that you can observe the blocking behavior. |
Contributor
|
OK I will try that. Thank Sincerely,
|
Contributor
Author
|
@gyuho My suggestion will be: Try with boltdb itself first
Then you can understand why we need this in etcd. When we might have a read routine (the snapshot sender that can take up to 1 minute for sending large snapshot), it might block writes for 10s of seconds. |
Contributor
|
OK got it. Will try. Thanks! Sincerely,
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am rewriting the snapshot merging and sending logic for v3 (#3666).
The previous approach is complicated and introduced a few unnecessary components (snapshot_store in both etcdserver and transport). And the coordination between components are error-prone. We have fixed a few bugs but there are still deadlocks around. Moreover the current snapshot creating logic is wrong. It does not stop the apply loop while getting the v3 snapshot.
The previous complexity comes from the delay of the snapshot merging at the final transportation phase. I try to move the merging to the very beginning at etcdserver and it makes thing a lot simper. It is mainly because etcdserver has the full control of the workflow and does not require any coordination.
This pull requests try to simplify the logic. Currently it is work in progress. But the approach works and is significantly simpler.
The snapshot sending logic should be as simple as: