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

Conversation

Projects
None yet
2 participants
@janardhan1993
Contributor

janardhan1993 commented Mar 26, 2018

This change is Reviewable

@janardhan1993

This comment has been minimized.

Show comment
Hide comment
@janardhan1993

janardhan1993 Mar 27, 2018

Contributor

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

Contributor

janardhan1993 commented Mar 27, 2018

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

This comment has been minimized.

Show comment
Hide comment
@pawanrawal

pawanrawal Mar 28, 2018

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

Contributor

pawanrawal commented Mar 28, 2018

: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

This comment has been minimized.

Show comment
Hide comment
@janardhan1993

janardhan1993 Mar 28, 2018

Contributor

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

Contributor

janardhan1993 commented Mar 28, 2018

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

1 of 3 checks passed

code-review/reviewable 1 file, 4 discussions left (pawanrawal)
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@janardhan1993 janardhan1993 deleted the fix/jepsen_delete branch Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment