-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
no-store_v2: Store ConfState as part of WAL log snapshot #12735
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12735 +/- ##
==========================================
- Coverage 72.76% 70.11% -2.65%
==========================================
Files 422 413 -9
Lines 33018 32063 -955
==========================================
- Hits 24024 22481 -1543
- Misses 7077 7681 +604
+ Partials 1917 1901 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
92817d7
to
2393c0d
Compare
func TestNew(t *testing.T) { | ||
p, err := ioutil.TempDir(os.TempDir(), "waltest") | ||
p, err := ioutil.TempDir(t.TempDir(), "waltest") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)
@@ -942,6 +941,10 @@ func (w *WAL) Save(st raftpb.HardState, ents []raftpb.Entry) error { | |||
} | |||
|
|||
func (w *WAL) SaveSnapshot(e walpb.Snapshot) error { | |||
if err := walpb.ValidateSnapshotForWrite(&e); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say a, b, c
And c as a follower runs this code against a as a leader with old etcd version. Would the follower reject the snapshot from old-versioned leader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there would be no problem as raft is speaking using raftpb.Snapshot
That always contains
Line 33 in 102c198
optional SnapshotMetadata metadata = 2 [(gogoproto.nullable) = false]; |
with:
Line 26 in 102c198
optional ConfState conf_state = 1 [(gogoproto.nullable) = false]; |
This validation just makes sure we never forgot to copy the content from raftpb.Snapshot to walpb.Snapshot.
That cannonically happens in server/etcdserver/storage.go (line 59)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ptabor +1. Yeah, this is for saving the snap locally.
2393c0d
to
917895e
Compare
@gyuho Shell I merge, or your concern remains ? |
/cc @xiang90 |
This will (among others) allow etcd-3.6 to not depend on store_v2 .snap files at all, as WAL + db file will be self-sufficient.
917895e
to
4d4c84e
Compare
WAL log snapshots used to contain only Term & Index.
That made them diverging from raft.Snapshot metadata that contained ConfState as well
(list of Voters, Listeners, and candidating voters & listeners).
So far lookup of the full ConfState required dive into *.snap file that is reminiscence of store_v2.
I'm working on preparing for decomissioning of store_v2 in 3.6, so in particular etcd-3.5
should be able to run in situation that *.snap files does not exist (to be rollback ready).