fix(GraphQL): Update GraphQL schema only on Group-1 leader

[abhimanyusinghgaur]

Fixes #GRAPHQL-364.

This PR changes the way how GraphQL schema is updated. From now on, every request to update the GraphQL schema will be executed on Group-1 leader, instead of on the alpha which received the request. This helps solve some concurrency issues during GraphQL schema update.

---

This change is 

Docs Preview:

[pawanrawal]

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

---

edgraph/server.go, line 130 at r1 (raw file):

}

func UpdateGQLSchema(ctx context.Context, gqlSchema,

Add a comment here.

---

edgraph/server.go, line 353 at r1 (raw file):

	// wait for indexing to complete or context to be canceled.
	if err = worker.WaitForIndexingOrCtxError(ctx, !op.RunInBackground); err != nil {
		return empty, ctx.Err()

return empty, err

---

graphql/e2e/schema/schema_test.go, line 177 at r1 (raw file):

	lastSuccessTcaseIdx := -1

	var mux sync.Mutex

mux := sync.Mutex{}
wg := sync.WaitGroup

---

graphql/e2e/schema/schema_test.go, line 256 at r1 (raw file):

	finalGraphQLSchema := tcases[lastSuccessTcaseIdx].graphQLSchema

	// now check that both the final GraphQL schema and Dgraph schema are the ones we expect

just run this a few times, say 100 in a loop and check that it doesn't fail randomly.

---

protos/pb.proto, line 643 at r1 (raw file):

message UpdateGraphQLSchemaRequest {
	uint64                startTs        = 1;

start_ts

---

protos/pb.proto, line 645 at r1 (raw file):

	uint64                startTs        = 1;
	string                graphql_schema = 2;
	repeated SchemaUpdate dgraph_schema  = 3;

dgraph_preds

---

worker/graphql_schema.go, line 68 at r1 (raw file):

	defer schemaLock.Unlock()

	var err error

Do we need to define it explictly here.

[martinmr]

Some minor comments but it once my comments and pawan's are addressed.

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

---

graphql/e2e/schema/schema_test.go, line 85 at r1 (raw file):

 see nodes

see if nodes

---

graphql/e2e/schema/schema_test.go, line 360 at r1 (raw file):

Quoted 5 lines of code…

func getNewDgraphClient(t *testing.T) *dgo.Dgraph {
	cc, err := grpc.Dial(groupOnegRPC, grpc.WithInsecure())
	require.NoError(t, err)
	return dgo.NewDgraphClient(api.NewDgraphClient(cc))
}

I think there's already a function in testutil that does this. It assumes the alpha is in 8180 which is the same address you are using.

---

systest/21million/queries/query-052, line 10 at r1 (raw file):

  "q": [
    {
      "count": 3508686

why did this change?

---

worker/graphql_schema.go, line 150 at r1 (raw file):

	//	providing every alpha a chance to reflect the current GraphQL schema before the response is
	//	sent back to the user.
	if _, err = CommitOverNetwork(ctx, tctx); err != nil {

just to clarify, you mentioned before that you were thinking about cancelling the mutation if the alter failed. It doesn't look like you are doing that anymore, is that correct?

---

worker/graphql_schema.go, line 187 at r1 (raw file):

}

func isGroup1Leader() bool {

isGroupOneLeader

Let's not put numbers in function names if we can avoid it.

[abhimanyusinghgaur]

Reviewable status: 3 of 13 files reviewed, 12 unresolved discussions (waiting on @manishrjain, @martinmr, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

edgraph/server.go, line 130 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment here.

Done.

---

edgraph/server.go, line 353 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

return empty, err

Done.

---

graphql/e2e/schema/schema_test.go, line 85 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

 see nodes

see if nodes

Done.

---

graphql/e2e/schema/schema_test.go, line 177 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

mux := sync.Mutex{}
wg := sync.WaitGroup

Done.

---

graphql/e2e/schema/schema_test.go, line 256 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

just run this a few times, say 100 in a loop and check that it doesn't fail randomly.

Done.
Verified with 100 runs in a loop, worked without errors.

---

graphql/e2e/schema/schema_test.go, line 360 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

func getNewDgraphClient(t *testing.T) *dgo.Dgraph {
	cc, err := grpc.Dial(groupOnegRPC, grpc.WithInsecure())
	require.NoError(t, err)
	return dgo.NewDgraphClient(api.NewDgraphClient(cc))
}

I think there's already a function in testutil that does this. It assumes the alpha is in 8180 which is the same address you are using.

Done.
Thanks

---

protos/pb.proto, line 643 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

start_ts

Done.

---

protos/pb.proto, line 645 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

dgraph_preds

Done.

---

systest/21million/queries/query-052, line 10 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

why did this change?

Previously, we used to upsert an empty node for GraphQL schema at alpha boot up, which doesn't happen anymore. So, the count has decreased by one.

---

worker/graphql_schema.go, line 68 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do we need to define it explictly here.

Done.

---

worker/graphql_schema.go, line 150 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

just to clarify, you mentioned before that you were thinking about cancelling the mutation if the alter failed. It doesn't look like you are doing that anymore, is that correct?

Yeah! that's what I was thinking earlier, but now doing the alter after committing the mutation has some benefits as mentioned in the comments. Also, alter can't fail here as we are validating the dgraph schema beforehand. If alter fails here, then it is a bug, for which I have put in the error message to ask users to report it on GitHub.
Also, even if alter fails, a user can just retry.

---

worker/graphql_schema.go, line 187 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

isGroupOneLeader

Let's not put numbers in function names if we can avoid it.

Done.