Skip to content

Update etcd to use the new raft module go.etcd.io/raft/v3 #14881

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

Merged
merged 17 commits into from
Dec 2, 2022

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Dec 2, 2022

Update etcd to use the new raft module go.etcd.io/raft/v3. Currently we depend on the latest commit of raft. When we release new tagged raft version, we can bump etcd by then.

The only left work is to update modules.svg. Of course, we also need to update the changelog. Both tasks will be done in separate PR(s).

Please note that the etcd/raft isn't removed, instead it is just marked as Deprecated for now. We can remove it in 3.7.

…aft/v3

Just replaced all go.etcd.io/etcd/raft/v3 with go.etcd.io/raft/v3
under directory server.

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr ahrtr marked this pull request as draft December 2, 2022 01:52
ahrtr added 12 commits December 2, 2022 12:59
Executed commands below,
1. Removed go.etcd.io/raft/v3 => ../raft;
2. go get go.etcd.io/raft/v3@eaa6808e1f7ab2247c13778250f70520b0527ff1
3. go mod tidy

Signed-off-by: Benjamin Wang <[email protected]>
gofmt -w ./server

Signed-off-by: Benjamin Wang <[email protected]>
Just replaced all go.etcd.io/etcd/raft/v3 with go.etcd.io/raft/v3
under directory etcdutl.

Signed-off-by: Benjamin Wang <[email protected]>
Executed commands below,
1. Removed go.etcd.io/raft/v3 => ../raft;
2. go get go.etcd.io/raft/v3@eaa6808e1f7ab2247c13778250f70520b0527ff1
3. go mod tidy

Signed-off-by: Benjamin Wang <[email protected]>
…io/raft/v3

Just replaced all go.etcd.io/etcd/raft/v3 with go.etcd.io/raft/v3
under tools/etcd-dump-logs.

Signed-off-by: Benjamin Wang <[email protected]>
Add go.mod and go.sum to fix issue below,
```
$ go build
../../server/etcdserver/api/snap/snapshotter.go:33:2: missing go.sum entry for module providing package go.etcd.io/raft/v3 (imported by go.etcd.io/etcd/server/v3/etcdserver/api/snap); to add:
	go get go.etcd.io/etcd/server/v3/etcdserver/api/[email protected]
../../server/storage/wal/walpb/record.pb.go:14:2: missing go.sum entry for module providing package go.etcd.io/raft/v3/raftpb (imported by go.etcd.io/etcd/v3/tools/etcd-dump-logs); to add:
	go get go.etcd.io/etcd/v3/tools/etcd-dump-logs
```

Signed-off-by: Benjamin Wang <[email protected]>
Just replaced all go.etcd.io/etcd/raft/v3 with go.etcd.io/raft/v3
under directory test.

Signed-off-by: Benjamin Wang <[email protected]>
Executed commands below,
1. Removed go.etcd.io/raft/v3 => ../raft;
2. go get go.etcd.io/raft/v3@eaa6808e1f7ab2247c13778250f70520b0527ff1;
3. go mod tidy

Signed-off-by: Benjamin Wang <[email protected]>
Executed commands below,
1. Removed go.etcd.io/raft/v3 => ../raft;
2. go get go.etcd.io/raft/v3@eaa6808e1f7ab2247c13778250f70520b0527ff1;
3. go mod tidy

Note that after execuing command `go mod tidy`, the dependency
"go.etcd.io/etcd/raft/v3 v3.5.6" was added automatically. When we
remove raft and the raftexample, and it will cleanup automatically
when executing `go mod tidy` again.

Signed-off-by: Benjamin Wang <[email protected]>
No need to generate proto file;
No need to test coverage for raft;
No need to run any test for raft module;
NO need to run any test for raftexample;

Signed-off-by: Benjamin Wang <[email protected]>
TODO:
1. Update Documentation/contributor-guide/modules.svg;
2. Update bill-of-materials.json when raft and raftexample are removed in future;

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr ahrtr force-pushed the update_raft_for_etcdserver_20221202 branch from 4e18efe to 8651478 Compare December 2, 2022 06:20
…raft/v3

Just replaced all go.etcd.io/etcd/raft/v3 with go.etcd.io/raft/v3
under directory contrib/raftexample.

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr ahrtr changed the title etcdserver: replace go.etcd.io/etcd/raft/v3 with go.etcd.io/raft/v3 Update etcd to use the new raft module go.etcd.io/raft/v3 Dec 2, 2022
ahrtr added 3 commits December 2, 2022 14:30
Just execute ./script/fix.sh after updating raftexample to use
the new raft module go.etcd.io/raft/v3.

Signed-off-by: Benjamin Wang <[email protected]>
Previously etcdservers depends on raft/raftpb/raft.proto directly.
After moving raft to a separate repo, we need to add raft to the
tools/mod, and get raft included in the -I protc flags.

Signed-off-by: Benjamin Wang <[email protected]>
Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr ahrtr marked this pull request as ready for review December 2, 2022 07:58
@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

cc @mitake @ptabor @serathius @spzala @tbg PTAL.

@ahrtr ahrtr added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Dec 2, 2022
@serathius
Copy link
Member

serathius commented Dec 2, 2022

Not sure about deprecation and removal in v3.7. It's a very long wait. I'm worried that wait doesn't have any benefits but could cause a lot of confusion for etcd contributors. Can be discussed in separate PR.

I don't see moving raft repo as breaking change. If someone users raft library they will just stop getting updates until they change package name. I see leaving this package in etcd repo rather as something that should be cleaned up. Based on how much technical depth is in etcd repo, I would prefer to pay it off earlier than later.

@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

Thanks both @ptabor and @serathius for the quick review.

I'm worried that wait doesn't have any benefits but could cause a lot of confusion for etcd contributors. Can be discussed in separate PR.

The only concern is that some applications might already depend on the raft on main, such as cockroachdb db (@tbg ). So we can't directly remove it. It's safer to remove it in 3.7.

Of course, we can discuss it separately.

@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

Based on how much technical depth is in etcd repo, I would prefer to pay it off earlier than later.

Personally I'd like to remove it soon. But at least, let's send formal notification and wait some time, e.g. 1 month? Nobody likes surprise.

@serathius
Copy link
Member

The only concern is that some applications might already depend on the raft on main, such as cockroachdb db (@tbg ). So we can't directly remove it. It's safer to remove it in 3.7.

You cannot depend on branches, you depend on commit hashes. Don't think cockroachdb makes a difference here.

@ahrtr ahrtr merged commit 26c0627 into etcd-io:main Dec 2, 2022
@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

You cannot depend on branches, you depend on commit hashes.

Right. If there is no any objection, Let me deliver a PR to remove it.

@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

Let's discuss in the PR.

@ahrtr ahrtr mentioned this pull request Dec 2, 2022
@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

#14882

@ahrtr
Copy link
Member Author

ahrtr commented Dec 2, 2022

Linked to #14713

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Development

Successfully merging this pull request may close these issues.

3 participants