Remove duplicate code for query processing

[mangalaman93]

The side effect of this is that now we return query response in an upsert.

This change is 

[martinmr]

Is this just a refactor? It looks like the response format also changes. The PR description should reflect that.

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @pawanrawal)

---

go.mod, line 17 at r2 (raw file):

	github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd
	github.com/dgraph-io/badger v0.0.0-20190917133922-cbdef65095c7
	github.com/dgraph-io/dgo/v2 v2.1.1-0.20191106095420-9d64a2d0ac17

Are all these changes to go.mod and go.sum needed for your PR?

---

dgraph/cmd/alpha/http.go, line 461 at r2 (raw file):

Quoted 53 lines of code…

	var out bytes.Buffer
	// If response is either empty or only contains empty braces {}, it's a no-op
	if len(resp.Json) > 2 {
		var i int
		for i = len(resp.Json) - 1; i >= 0; i-- {
			if resp.Json[i] == '}' {
				break
			}
		}

		out = *bytes.NewBuffer(resp.Json[:i])
		if _, err := out.WriteRune(','); err != nil {
			x.SetStatusWithData(w, x.Error, err.Error())
			return
		}
	} else {
		if _, err := out.WriteRune('{'); err != nil {
			x.SetStatusWithData(w, x.Error, err.Error())
			return
		}

	}


	if err := writeEntry(&out, "code", []byte("\""+x.Success+"\"")); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if _, err := out.WriteRune(','); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if err := writeEntry(&out, "message", []byte("\"Done\"")); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if _, err := out.WriteRune(','); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	uids, err := json.Marshal(resp.Uids)
	if err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if err := writeEntry(&out, "uids", uids); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if _, err := out.WriteRune('}'); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}

Why are we manually writing the response here instead of doing something like what was before. It's pretty hard to understand and prone to bugs.

---

edgraph/server.go, line 899 at r2 (raw file):

	// TODO(martinmr): Include Transport as part of the latency. Need to do
	// this separately since it involves modifying the API protos.
	l.Processing = time.Since(l.Start) - l.Parsing - l.AssignTimestamp - l.Json

This works now but is kind of brittle. It depends on now new fields being added. I would look into a better way to define the processing time.

[martinmr]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @pawanrawal)

---

dgraph/cmd/alpha/http.go, line 461 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	var out bytes.Buffer
	// If response is either empty or only contains empty braces {}, it's a no-op
	if len(resp.Json) > 2 {
		var i int
		for i = len(resp.Json) - 1; i >= 0; i-- {
			if resp.Json[i] == '}' {
				break
			}
		}

		out = *bytes.NewBuffer(resp.Json[:i])
		if _, err := out.WriteRune(','); err != nil {
			x.SetStatusWithData(w, x.Error, err.Error())
			return
		}
	} else {
		if _, err := out.WriteRune('{'); err != nil {
			x.SetStatusWithData(w, x.Error, err.Error())
			return
		}

	}


	if err := writeEntry(&out, "code", []byte("\""+x.Success+"\"")); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if _, err := out.WriteRune(','); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if err := writeEntry(&out, "message", []byte("\"Done\"")); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if _, err := out.WriteRune(','); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	uids, err := json.Marshal(resp.Uids)
	if err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if err := writeEntry(&out, "uids", uids); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}
	if _, err := out.WriteRune('}'); err != nil {
		x.SetStatusWithData(w, x.Error, err.Error())
		return
	}

Why are we manually writing the response here instead of doing something like what was before. It's pretty hard to understand and prone to bugs.

Nevermind, I see Manish suggested this to make it faster.

I would then look into trying to have this code into its own function and add unit tests to make sure that the final output is correct JSON.

[pawanrawal]

Appreciate the effort put into refactoring this piece of code. I have some questions, the most important one being about BestEffort, apart from that this looks good.

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @mangalaman93 and @manishrjain)

---

dgraph/cmd/alpha/http.go, line 434 at r2 (raw file):

	}

	if err := writeEntry(&out, "code", []byte("\""+x.Success+"\"")); err != nil {

A lot of this repetitive error checking could be simplified i.e. made less verbose using one of the patterns described in https://blog.golang.org/errors-are-values. Please see if it makes sense to use that.

---

edgraph/server.go, line 464 at r2 (raw file):

	updateMutations(qc)

	gmu := qc.gmuList[0]

Would there ever be more than one gmuList? I thought that you were going to collect sets and deletes across all mutations and put them in the same gmuList.

---

edgraph/server.go, line 575 at r2 (raw file):

}

func updateMutations(qc *queryContext) {

Some comments here about what this function does would help others who are not very familiar with the code.

---

edgraph/server.go, line 579 at r2 (raw file):

		gmu := qc.gmuList[i]
		if len(condVar) != 0 {
			uids, ok := qc.uidRes[condVar]

Would this ever be !ok? Or are you just checking it for safety?

---

edgraph/server.go, line 593 at r2 (raw file):

// findVars finds all the variables used in mutation block
func findVars(qc *queryContext) []string {

This function name is a bit misleading as it also updates the vars apart from finding them. Do you know why does it update them to nil? Should that happen somewhere else?

---

edgraph/server.go, line 880 at r2 (raw file):

	if qc.req.StartTs == 0 {
		start := time.Now()
		if qc.req.BestEffort {

Since we have a common function now for queries and mutations, can a user send just a mutation that is BestEffort? If yes, then I am not sure if that is right or something that we should allow. I might be wrong here but my understanding was that BestEffort is only for queries.

---

edgraph/server.go, line 1031 at r2 (raw file):

		qc.valRes = make(map[string]map[uint64]types.Val)
		upsertQuery = buildUpsertQuery(qc)
		needVars = findVars(qc)

This should maybe called findMutationVars or something since qc now has both query and mutation.

[mangalaman93]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

go.mod, line 17 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Are all these changes to go.mod and go.sum needed for your PR?

I need the dgo change, rest just seems a side effect of go mod tidy. I will remove my changes and see if some of the changes go away.

---

dgraph/cmd/alpha/http.go, line 434 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

A lot of this repetitive error checking could be simplified i.e. made less verbose using one of the patterns described in https://blog.golang.org/errors-are-values. Please see if it makes sense to use that.

I would prefer that I do this in a different PR. This seems a perfect use case for this. I can refactor both queryHandler and mutationHandler to take advantage of the new type I define.

---

dgraph/cmd/alpha/http.go, line 461 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Nevermind, I see Manish suggested this to make it faster.

I would then look into trying to have this code into its own function and add unit tests to make sure that the final output is correct JSON.

I will just refactor it how @pawanrawal suggested.

---

edgraph/server.go, line 464 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Would there ever be more than one gmuList? I thought that you were going to collect sets and deletes across all mutations and put them in the same gmuList.

yes, we do the merging in the ToDirectedEdges function. gmuList is a one to one mapping from the mutation to its corresponding parsed mutation. (api.Mutation)

---

edgraph/server.go, line 575 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Some comments here about what this function does would help others who are not very familiar with the code.

Done.

---

edgraph/server.go, line 579 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Would this ever be !ok? Or are you just checking it for safety?

Safety only.

---

edgraph/server.go, line 593 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This function name is a bit misleading as it also updates the vars apart from finding them. Do you know why does it update them to nil? Should that happen somewhere else?

It stores the variable list in uidRes and valRes so that when we look at variables query results, we only extract values for variables that we need. Earlier we would iterate and store all the variables defined in the query of upsert even when a variable is not used in the mutation. Let me know if you still think that the function name is misleading. I will add this information as a comment in the code.

---

edgraph/server.go, line 880 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Since we have a common function now for queries and mutations, can a user send just a mutation that is BestEffort? If yes, then I am not sure if that is right or something that we should allow. I might be wrong here but my understanding was that BestEffort is only for queries.

Valid point, fixed this now.

---

edgraph/server.go, line 899 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

This works now but is kind of brittle. It depends on now new fields being added. I would look into a better way to define the processing time.

@manishrjain do you have suggestions on what the right definition of processing time is.

---

edgraph/server.go, line 1031 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This should maybe called findMutationVars or something since qc now has both query and mutation.

Done.

[pawanrawal]

Reviewed 2 of 3 files at r3.
Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

[manishrjain]

Reviewable status: 8 of 9 files reviewed, 8 unresolved discussions (waiting on @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

dgraph/cmd/alpha/http.go, line 161 at r3 (raw file):

func writeEntry(out *bytes.Buffer, key string, js []byte) error {
	if _, err := out.WriteRune('"'); err != nil {

Writes to bytes.Buffer always return nil. So, no need. Does not need to return an error.

---

dgraph/cmd/alpha/http.go, line 417 at r3 (raw file):

		var i int
		for i = len(resp.Json) - 1; i >= 0; i-- {
			if resp.Json[i] == '}' {

if resp.Json[i] == ' ' continue
else if resp.Json[i] == '}' break
else return error?

Or, just check for the last one to be right brace.

---

dgraph/cmd/alpha/http.go, line 422 at r3 (raw file):

		}

		out = *bytes.NewBuffer(resp.Json[:i])

out = bytes.NewBuffer(...)

Then don't need to do this &out below.

---

dgraph/cmd/alpha/http.go, line 434 at r3 (raw file):

	}

	if err := writeEntry(&out, "code", []byte("\""+x.Success+"\"")); err != nil {

Remove all these error handling here.

x.Check(out.Write...)
_ = out.Write

out.Write // golinter would complain.

[manishrjain]

Got some comments. Leave it to others to approve.

Reviewed 6 of 9 files at r1, 3 of 3 files at r3.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @mangalaman93 and @martinmr)

---

dgraph/cmd/alpha/http.go, line 417 at r3 (raw file):

	mp["code"] = x.Success
	mp["message"] = "Done"
	mp["uids"] = resp.Uids

I'd do json.Marshal here, and then plug that output into the previous response.

---

dgraph/cmd/alpha/http.go, line 434 at r3 (raw file):

	}

	if err := writeEntry(&out, "code", []byte("\""+x.Success+"\"")); err != nil {

out.Write(`"code": "`)
out.Write(x.Success)
out.Write(`"`)

---

edgraph/server.go, line 899 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

@manishrjain do you have suggestions on what the right definition of processing time is.

Yeah, processing time should be calculated, not derived.

---

edgraph/server.go, line 889 at r3 (raw file):

			qc.req.StartTs = posting.Oracle().MaxAssigned()
		} else {
			qc.req.StartTs = State.getTimestamp(qc.req.ReadOnly)

Bring this StartTs logic back to what it was.

---

edgraph/server.go, line 1015 at r3 (raw file):

	}

	if req.StartTs == 0 {

Looks like this logic got changed in this refactoring.

---

edgraph/server.go, line 1015 at r3 (raw file):

// parseRequest parses the incoming request
func parseRequest(qc *queryContext) error {
	parsingStartTime := time.Now()

startTs
// start

---

edgraph/server.go, line 1068 at r3 (raw file):

	}

	if len(qc.gmuList) > 0 {

for _, list := range qc.gmuList {
//
}

or, add a TODO.

[mangalaman93]

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @martinmr)

---

dgraph/cmd/alpha/http.go, line 461 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I will just refactor it how @pawanrawal suggested.

Not needed anymore

---

dgraph/cmd/alpha/http.go, line 161 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Writes to bytes.Buffer always return nil. So, no need. Does not need to return an error.

Done.

---

dgraph/cmd/alpha/http.go, line 417 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if resp.Json[i] == ' ' continue
else if resp.Json[i] == '}' break
else return error?

Or, just check for the last one to be right brace.

Done.

---

dgraph/cmd/alpha/http.go, line 417 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd do json.Marshal here, and then plug that output into the previous response.

Done.

---

dgraph/cmd/alpha/http.go, line 422 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

out = bytes.NewBuffer(...)

Then don't need to do this &out below.

Done.

---

dgraph/cmd/alpha/http.go, line 434 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove all these error handling here.

x.Check(out.Write...)
_ = out.Write

out.Write // golinter would complain.

Done.

---

dgraph/cmd/alpha/http.go, line 434 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

out.Write(`"code": "`)
out.Write(x.Success)
out.Write(`"`)

Done.

---

edgraph/server.go, line 1015 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

startTs
// start

Done.

---

edgraph/server.go, line 1068 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

for _, list := range qc.gmuList {
//
}

or, add a TODO.

Done.

[golangcibot]

S1021: should merge variable declaration with assignment on next line (from gosimple)

[mangalaman93]

Reviewable status: 3 of 8 files reviewed, 15 unresolved discussions (waiting on @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/server.go, line 899 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Yeah, processing time should be calculated, not derived.

Done.

---

edgraph/server.go, line 889 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Bring this StartTs logic back to what it was.

Done.

---

edgraph/server.go, line 1015 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Looks like this logic got changed in this refactoring.

Done.

[manishrjain]

Just a few comments, maybe for different PRs.

Reviewed 3 of 6 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @martinmr)

---

dgraph/cmd/alpha/http.go, line 410 at r6 (raw file):

	}

	js, err := json.Marshal(response)

json.NewEncoder(w)
if err := enc.Marshal(response); err != nil { log it }

---

edgraph/server.go, line 552 at r6 (raw file):

	isCondUpsert := strings.TrimSpace(gmu.Cond) != ""
	if isCondUpsert {
		qc.condVars[0] = fmt.Sprintf("__dgraph_%d__", rand.Int())

Just use a local counter here.
"__dgraph_" + IToa(counter)