Skip to content
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

raft: check pending conf change before campaign #12134

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

BusyJay
Copy link
Contributor

@BusyJay BusyJay commented Jul 14, 2020

fix #12133.

raft/raft.go Show resolved Hide resolved
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #12134 into master will decrease coverage by 0.67%.
The diff coverage is 92.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12134      +/-   ##
==========================================
- Coverage   63.96%   63.28%   -0.68%     
==========================================
  Files         403      403              
  Lines       37369    37389      +20     
==========================================
- Hits        23902    23661     -241     
- Misses      11942    12199     +257     
- Partials     1525     1529       +4     
Impacted Files Coverage Δ
raft/raft.go 91.09% <92.00%> (+0.71%) ⬆️
client/keys.go 55.77% <0.00%> (-35.68%) ⬇️
auth/store.go 53.67% <0.00%> (-21.47%) ⬇️
auth/range_perm_cache.go 54.28% <0.00%> (-14.29%) ⬇️
proxy/grpcproxy/register.go 75.00% <0.00%> (-7.50%) ⬇️
pkg/fileutil/purge.go 69.56% <0.00%> (-6.53%) ⬇️
pkg/testutil/recorder.go 77.77% <0.00%> (-3.71%) ⬇️
etcdserver/api/v3rpc/interceptor.go 69.18% <0.00%> (-3.49%) ⬇️
clientv3/leasing/util.go 95.00% <0.00%> (-3.34%) ⬇️
etcdserver/api/v3election/election.go 61.11% <0.00%> (-2.78%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c6881f...54ba41c. Read the comment docs.

@xiang90
Copy link
Contributor

xiang90 commented Jul 21, 2020

@BusyJay Create a separate PR for the 3rd commit since it is not relevant to raft: check pending conf change before campaign nor https://github.com/etcd-io/etcd/issues/12133? Otherwise, lgtm.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
Copy link
Contributor Author

BusyJay commented Jul 22, 2020

The commit is removed.

@xiang90
Copy link
Contributor

xiang90 commented Jul 22, 2020

lgtm

@xiang90 xiang90 merged commit d0e4fe5 into etcd-io:master Jul 23, 2020
@BusyJay BusyJay deleted the fix-12133 branch July 23, 2020 01:54
}
ents, err := r.raftLog.slice(r.raftLog.applied+1, r.raftLog.committed+1, noLimit)
if err != nil {
r.logger.Panicf("unexpected error getting unapplied entries (%v)", err)
Copy link
Contributor

@cfc4n cfc4n Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there need return err ?

I can't pass unit tests with error
https://travis-ci.com/github/etcd-io/etcd/jobs/363601925#L3846

raft2020/07/23 19:40:17 unexpected error getting unapplied entries (requested index is unavailable due to compaction)
--- FAIL: TestLeaderTransferAfterSnapshot (0.00s)
panic: unexpected error getting unapplied entries (requested index is unavailable due to compaction) [recovered]
	panic: unexpected error getting unapplied entries (requested index is unavailable due to compaction)

goroutine 213 [running]:
testing.tRunner.func1.1(0xba71e0, 0xc000055bc0)
	/usr/local/go1.14.4/go/src/testing/testing.go:940 +0x421
testing.tRunner.func1(0xc000477680)
	/usr/local/go1.14.4/go/src/testing/testing.go:943 +0x600
panic(0xba71e0, 0xc000055bc0)
	/usr/local/go1.14.4/go/src/runtime/panic.go:975 +0x3e3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BusyJay Can you review this comment , It block my PR test ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an error that needs to be fixed by the removed commit. I will send it as a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is sent as #12163.

craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Nov 13, 2020
56233: *: upgrade etcd/raft to master r=knz a=tbg

We were using a custom fork that we picked up during our vgo transition,
but in the meantime upstream has turned `raft` into a small module which
has very few deps, and which we can switch back to.

Helpfully, this module already pins gogoproto v1.3+, which simplifies
PR #56147.

I did review the commits we're picking up here in etcd/raft and they look good. The ones worth pointing out are:

etcd-io/etcd#12137
etcd-io/etcd#12134
etcd-io/etcd#12163

These all seem to fix real bugs.

Release note: None


Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
absolute8511 added a commit to youzan/ZanRedisDB that referenced this pull request Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Pending conf change is not handled correctly during campaign
4 participants