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

Start adding documentation and fix data races in query.go. #3749

Merged
merged 10 commits into from Aug 6, 2019

Conversation

@pawanrawal
Copy link
Member

commented Aug 5, 2019

  1. Add comments explaining what various fields in SubGraph are used for and what are the functions doing.
  2. This change also fixes the remaining race conditions mentioned in #3685.
  3. Moves preTraverse related code to outputnode.go as it doesn't deal with the query execution and query.go is very long(3k lines).

This change is Reviewable

Order []*pb.Order // List of predicates to sort by and the sort order.
// TODO - Is Var only populated when this subgraph defines a variable or even when it needs it?
Var string // The name of the variable required by this SubGraph.
NeedsVar []gql.VarContext // The list of variables required by this SubGraph along with their type which can be uid or value.

This comment has been minimized.

Copy link
@golangcibot

golangcibot Aug 5, 2019

line is 128 characters (from lll)

@pawanrawal pawanrawal changed the title Start adding comments and fix data races in query.go. Start adding documentation and fix data races in query.go. Aug 5, 2019
Copy link
Member Author

left a comment

There are still a lot more comments that I want to add which I'll do over time.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @golangcibot)


query/outputnode.go, line 527 at r2 (raw file):

	return bufw.Bytes(), nil
}

All the code in this field has been moved from query.go without change, hence doesn't need to be looked at carefully.


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

Previously, golangcibot (Bot from GolangCI) wrote…

line is 128 characters (from lll)

Done.


query/query.go, line 1409 at r2 (raw file):

	if len(sg.Params.FacetVar) == 0 || sg.Params.Facet == nil {
		return nil
	}

Reversing the conditions to reduce the indentation of the code.


query/query.go, line 1546 at r2 (raw file):

	for _, v := range sg.Params.NeedsVar {
		l, ok := mp[v.Name]
		if !ok {

This allows us to reduce the nesting of the code below.


worker/task.go, line 48 at r2 (raw file):

var (
	emptyUIDList    pb.List

These global variables were the cause of the race condition since they are shared between different concurrent queries.

@pawanrawal pawanrawal marked this pull request as ready for review Aug 5, 2019
@pawanrawal pawanrawal requested review from manishrjain and dgraph-io/team as code owners Aug 5, 2019
Copy link
Member

left a comment

Nice change!

Can you confirm there's no logical changes, so I don't need to review that deeply. Also, once it's merged, can you run flock (ask Aman how to), keep it running, so you can identify any issues.

Reviewed 1 of 2 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @golangcibot)

Copy link
Member

left a comment

:lgtm:

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @golangcibot)

Var string
NeedsVar []gql.VarContext
Alias string
Count int // Value of first parameter.

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

The word first could be confusing. I initially thought there was a sequence of parameters somewhere, and first meant the first of those parameters. Could quote it may be.

FacetOrder string
FacetOrderDesc bool

// The name of the defined by this SubGraph.

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

Something is missing, probably meant defined variable

@@ -111,51 +110,76 @@ type Latency struct {
Json time.Duration `json:"json_conversion"`
}

// params contains the list of parameters required to execute a query.

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

execute a Subgraph may be?

@@ -174,14 +198,29 @@ type Function struct {
// query and the response. Once generated, this can then be encoded to other
// client convenient formats, like GraphQL / JSON.
type SubGraph struct {

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

query and subgraph are interchangeably used. Not sure whether it makes sense to keep the distinction (or there is one)

// execute this query.
Params params

// count stores the count of a predicate. There would be one value corresponding to each uid

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

What does count of a predicate mean?

@@ -1104,11 +860,14 @@ func createTaskQuery(sg *SubGraph) (*pb.Query, error) {
return out, nil
}

// varValue is a generic representation of a variable and holds multiple things.
// TODO - Come back to this and document what do individual fields mean and when are they populated.

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

Please add TODO(name) everywhere.

for _, uid := range sg.SrcUIDs.Uids {
curVal, ok := sg.Params.uidToVal[uid]
if ok && types.CompareVals(sg.SrcFunc.Name, curVal, dst) {
sg.DestUIDs.Uids = append(sg.DestUIDs.Uids, uid)
}
}
} else {
// This means its a root as SrcUIDs is nil
// This means its a function at root as SrcUIDs is nil

This comment has been minimized.

Copy link
@mangalaman93

mangalaman93 Aug 5, 2019

Contributor

it's

Copy link
Contributor

left a comment

Can't run flock without the desktop. @animesh2049 could you run flock and check for race conditions? Happy to sit with you if needed.

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @golangcibot and @pawanrawal)

Copy link
Member Author

left a comment

There is just one logical change which calls fillVars with sg.Params.ParentVars only if the current SubGraph has any children. That's a minor optimization. Apart from that there are no other logical changes.

I did run flock for 30 mins before my laptop killed the process and couldn't observe the race happening anymore. It happens in 5 mins without the change. Though would be nice for @animesh2049 to verify as well.

Reviewable status: 2 of 3 files reviewed, 12 unresolved discussions (waiting on @golangcibot, @mangalaman93, and @manishrjain)


query/query.go, line 113 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

execute a Subgraph may be?

Done.


query/query.go, line 116 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

The word first could be confusing. I initially thought there was a sequence of parameters somewhere, and first meant the first of those parameters. Could quote it may be.

Done


query/query.go, line 130 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Something is missing, probably meant defined variable

Done.


query/query.go, line 200 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

query and subgraph are interchangeably used. Not sure whether it makes sense to keep the distinction (or there is one)

Done. That was a pretty old comment.


query/query.go, line 209 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

What does count of a predicate mean?

Is count of an edge better?


query/query.go, line 864 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Please add TODO(name) everywhere.

Done.


query/query.go, line 1668 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

it's

Done.

@pawanrawal pawanrawal merged commit 8ba4a85 into master Aug 6, 2019
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 1 file, 12 discussions left (golangcibot, mangalaman93, manishrjain)
Details
Blockade (dgraph) TeamCity build finished
Details
CI (dgraph) TeamCity build finished
Details
GolangCI No issues found!
Details
license/cla Contributor License Agreement is signed.
Details
@pawanrawal pawanrawal deleted the query-docs branch Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.