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

Correct lease with check quorum. #141

Merged
merged 6 commits into from
Nov 30, 2018
Merged

Conversation

Hoverbear
Copy link
Contributor

Fixes #140 .

You can see etcd also has this behavior: https://github.com/etcd-io/etcd/blob/83304cfc808cf6303d48c45a696f169fae422e68/raft/raft.go#L1010-L1037

	case pb.MsgReadIndex:
		if r.quorum() > 1 {
			if r.raftLog.zeroTermOnErrCompacted(r.raftLog.term(r.raftLog.committed)) != r.Term {
				// Reject read only request when this leader has not committed any log entry at its term.
				return nil
			}

			// thinking: use an interally defined context instead of the user given context.
			// We can express this in terms of the term and index instead of a user-supplied value.
			// This would allow multiple reads to piggyback on the same message.
			switch r.readOnly.option {
			case ReadOnlySafe:
				r.readOnly.addRequest(r.raftLog.committed, m)
				r.bcastHeartbeatWithCtx(m.Entries[0].Data)
			case ReadOnlyLeaseBased:
				ri := r.raftLog.committed
				if m.From == None || m.From == r.id { // from local member
					r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
				} else {
					r.send(pb.Message{To: m.From, Type: pb.MsgReadIndexResp, Index: ri, Entries: m.Entries})
				}
			}
		} else {
			r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
		}

		return nil
}

@Hoverbear Hoverbear added the Bug Recognized misbehavior. label Nov 10, 2018
@Hoverbear Hoverbear self-assigned this Nov 10, 2018
@siddontang
Copy link
Contributor

PTAL @xiang90

@siddontang
Copy link
Contributor

LGTM

BusyJay
BusyJay previously approved these changes Nov 13, 2018
@Hoverbear

This comment has been minimized.

@bors

This comment has been minimized.

@Hoverbear

This comment has been minimized.

bors bot added a commit that referenced this pull request Nov 13, 2018
141: Correct lease with check quorum. r=Hoverbear a=Hoverbear

Fixes #140 .

You can see etcd also has this behavior: https://github.com/etcd-io/etcd/blob/83304cfc808cf6303d48c45a696f169fae422e68/raft/raft.go#L1010-L1037

```go
	case pb.MsgReadIndex:
		if r.quorum() > 1 {
			if r.raftLog.zeroTermOnErrCompacted(r.raftLog.term(r.raftLog.committed)) != r.Term {
				// Reject read only request when this leader has not committed any log entry at its term.
				return nil
			}

			// thinking: use an interally defined context instead of the user given context.
			// We can express this in terms of the term and index instead of a user-supplied value.
			// This would allow multiple reads to piggyback on the same message.
			switch r.readOnly.option {
			case ReadOnlySafe:
				r.readOnly.addRequest(r.raftLog.committed, m)
				r.bcastHeartbeatWithCtx(m.Entries[0].Data)
			case ReadOnlyLeaseBased:
				ri := r.raftLog.committed
				if m.From == None || m.From == r.id { // from local member
					r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
				} else {
					r.send(pb.Message{To: m.From, Type: pb.MsgReadIndexResp, Index: ri, Entries: m.Entries})
				}
			}
		} else {
			r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
		}

		return nil
}
```

Co-authored-by: Hoverbear <operator@hoverbear.org>
Co-authored-by: A. Hobden <operator@hoverbear.org>
@Hoverbear

This comment has been minimized.

bors bot added a commit that referenced this pull request Nov 13, 2018
141: Correct lease with check quorum. r=Hoverbear a=Hoverbear

Fixes #140 .

You can see etcd also has this behavior: https://github.com/etcd-io/etcd/blob/83304cfc808cf6303d48c45a696f169fae422e68/raft/raft.go#L1010-L1037

```go
	case pb.MsgReadIndex:
		if r.quorum() > 1 {
			if r.raftLog.zeroTermOnErrCompacted(r.raftLog.term(r.raftLog.committed)) != r.Term {
				// Reject read only request when this leader has not committed any log entry at its term.
				return nil
			}

			// thinking: use an interally defined context instead of the user given context.
			// We can express this in terms of the term and index instead of a user-supplied value.
			// This would allow multiple reads to piggyback on the same message.
			switch r.readOnly.option {
			case ReadOnlySafe:
				r.readOnly.addRequest(r.raftLog.committed, m)
				r.bcastHeartbeatWithCtx(m.Entries[0].Data)
			case ReadOnlyLeaseBased:
				ri := r.raftLog.committed
				if m.From == None || m.From == r.id { // from local member
					r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
				} else {
					r.send(pb.Message{To: m.From, Type: pb.MsgReadIndexResp, Index: ri, Entries: m.Entries})
				}
			}
		} else {
			r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
		}

		return nil
}
```

Co-authored-by: Hoverbear <operator@hoverbear.org>
Co-authored-by: A. Hobden <operator@hoverbear.org>
@Hoverbear
Copy link
Contributor Author

Oh, @siddontang Please give a real LGTM (a review instead of a comment). :) bors won't merge since only one review.

@nolouch
Copy link
Contributor

nolouch commented Nov 14, 2018

@Hoverbear Should we check the CheckQurom is enabled in the LeaseBase request? like: etcd-io/etcd@9801fd7

@Hoverbear
Copy link
Contributor Author

@nolouch I think you're right! I'll reflect that here.

@Hoverbear
Copy link
Contributor Author

@nolouch PTAL again! :)

Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

LGTM

@Hoverbear
Copy link
Contributor Author

@siddontang ping, you said LGTM but did not actually approve it.

@Hoverbear
Copy link
Contributor Author

@siddontang Friendly Ping

@nolouch nolouch merged commit 40157d8 into master Nov 30, 2018
@nolouch nolouch deleted the correct-lease-with-check-quorum branch November 30, 2018 12:33
@Hoverbear Hoverbear added this to the 0.5.0 milestone Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Recognized misbehavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants