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

Fix remaning jepsen issues #2261

Merged
merged 2 commits into from
Mar 28, 2018
Merged

Fix remaning jepsen issues #2261

merged 2 commits into from
Mar 28, 2018

Conversation

janardhan1993
Copy link
Contributor

@janardhan1993 janardhan1993 commented Mar 26, 2018

This change is Reviewable

@janardhan1993
Copy link
Contributor Author

Will remove linearizability from this PR. Kept it for testing


Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.


dgraph/cmd/zero/raft.go, line 574 at r2 (raw file):

					n.server.updateLeases()
				}
				leader = rd.RaftState == raft.StateLeader

There was a bug here, we weren't setting leader to false when it steps down. So once a node steps down and then becomes leader again the lease was continuing from the leader was leader. (So readTs was going back)


posting/list.go, line 348 at r1 (raw file):

		l.markdeleteAll > 0 && t.Op == intern.DirectedEdge_DEL &&
		bytes.Equal(t.Value, []byte(x.Star))
	doAbort := hasPendingDelete || txn.StartTs < l.commitTs

This needn't be true always for index keys, leave conflict detection to zero shouldn't do here.

Example:
<0x1> "abc" . say startts 1 committs 2
<0x2> "abc" , startts 3 committs 4
now for index key "abc" anything can get applied first
on replica
because scheduler does dependency checking only at sp level
even if we don't do here zero would do conflict detection
Only difference is user would get error while committing instead of mutating


worker/draft.go, line 125 at r1 (raw file):

var errReadIndex = x.Errorf("cannot get linerized read (time expired or no configured leader)")

func (n *node) WaitLinearizableRead(ctx context.Context) error {

Copied linerizability implementation from zero/raft.go to check whether it fixes the issue or not.
Will change it to batching implementation


Comments from Reviewable

@pawanrawal
Copy link
Contributor

:lgtm: some nice fixes there.


Reviewed 1 of 5 files at r1, 1 of 2 files at r2, 3 of 4 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


dgraph/cmd/zero/raft.go, line 574 at r2 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

There was a bug here, we weren't setting leader to false when it steps down. So once a node steps down and then becomes leader again the lease was continuing from the leader was leader. (So readTs was going back)

good one.


worker/mutation.go, line 416 at r3 (raw file):

	if txn := posting.Txns().Get(startTs); txn != nil {
		txn.Fill(tctx)
		if l := len(txn.Indices); l > 0 {

Can we use txn.LastIndex() here? Also, can that function have a RLock instead of Lock?


Comments from Reviewable

@janardhan1993
Copy link
Contributor Author

Review status: 4 of 5 files reviewed at latest revision, 4 unresolved discussions.


worker/mutation.go, line 416 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we use txn.LastIndex() here? Also, can that function have a RLock instead of Lock?

Done.


Comments from Reviewable

@janardhan1993 janardhan1993 merged commit 978c749 into master Mar 28, 2018
@janardhan1993 janardhan1993 deleted the fix/jepsen_delete branch March 28, 2018 06:50
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.

2 participants