Use txn.Get in addReverseMutation

[animesh2049]

We cannot use txn.cache.GetFromDeltas for reverse mutations becasue
cache will not have deltas corresponding to reverse key. Fixes #3840

---

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

@animesh2049 you can click here to see the review status or cancel the code review job.

[pullrequest]

This change looks good to me assuming the tests are all passing. Nice work finding this and adding a test that would have caught the issue.

---

Reviewed with ❤️ by PullRequest

[pullrequest]

Would it be worth adding the comment from your description here in the code just to prevent someone from wondering if this can use GetFromDelta in the future?

[gitlw]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

---

dgraph/cmd/alpha/upsert_test.go, line 1434 at r1 (raw file):

	m1 := `
{
set {

It seems like the data set can be simplified to just between two nodes 2 and 3.

---

posting/index.go, line 172 at r1 (raw file):

func (txn *Txn) addReverseMutation(ctx context.Context, t *pb.DirectedEdge) error {
	key := x.ReverseKey(t.Attr, t.ValueId)
	plist, err := txn.Get(key)

Inside the runMutation function, there is a switch block to determine whether we should use the txn.Get or txn.GetFromDelta,
I feel this decision stored in the current getFn variable should be stored as a variable in the Txn object, and then used in all the other places where txn.Get or txn.GetFromDelta are required.

It's better for the decision to be concentrated to one place, so as to avoid random bugs like this.

[manishrjain]

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gitlw, @martinmr, and @pullrequest[bot])

---

dgraph/cmd/alpha/upsert_test.go, line 1434 at r1 (raw file):

Previously, gitlw (Lucas Wang) wrote…

It seems like the data set can be simplified to just between two nodes 2 and 3.

Can you follow up with Animesh to get these addressed?

---

posting/index.go, line 172 at r1 (raw file):

Previously, gitlw (Lucas Wang) wrote…

Inside the runMutation function, there is a switch block to determine whether we should use the txn.Get or txn.GetFromDelta,
I feel this decision stored in the current getFn variable should be stored as a variable in the Txn object, and then used in all the other places where txn.Get or txn.GetFromDelta are required.

It's better for the decision to be concentrated to one place, so as to avoid random bugs like this.

We only have that knowledge here, because this logic knows if we're going to do the count index or not. Though, if you can think of a way to consolidate all that into one place, do raise a PR. For now, this seemed good enough (getting the release candidate out).

---

posting/index.go, line 172 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Would it be worth adding the comment from your description here in the code just to prevent someone from wondering if this can use GetFromDelta in the future?

Added comments.