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

GraphQL Admin API: Support export, draining, shutdown and setting lru_mb operations #4739

Merged
merged 9 commits into from
Feb 7, 2020

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented Feb 6, 2020

This PR adds support for doing export, shutdown and draining using the GraphQL Admin API. The mutations are shown below. The current /admin HTTP endpoints have been kept as before.

Export

mutation {
  export(input: {format: "json"}) {
    response {
      code
      message
    }
  }
}

Draining

mutation {
  draining(input: {enable: false}) {
    response {
      code
      message
    }
  }
}

Shutdown

mutation {
  shutdown {
    response {
      code
      message
    }
  }
}

Config(lru_mb)

mutation {
  config(input: {lruMb: 1025}) {
    response {
      code
      message
    }
  }
}

This change is Reviewable

@pawanrawal pawanrawal requested review from manishrjain and a team as code owners February 6, 2020 12:41
@pawanrawal pawanrawal changed the title GraphQL Admin API: Support export and draining operations GraphQL Admin API: Support export, draining and shutdown operations Feb 6, 2020
@pawanrawal pawanrawal changed the title GraphQL Admin API: Support export, draining and shutdown operations GraphQL Admin API: Support export, draining, shutdown and setting lru_mb operations Feb 6, 2020
@pawanrawal pawanrawal added the area/graphql Issues related to GraphQL support on Dgraph. label Feb 6, 2020
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment but the changes in Dgraph look good. I didn't review the graphQL part since I am not very familiar with it.

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


worker/groups.go, line 1084 at r1 (raw file):

			if err != nil {
				glog.Errorf("Error from alpha client subscribe: %v", err)
				time.Sleep(100 * time.Millisecond)

I am assuming this is done to avoid hogging the CPU, right?


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

	}

	posting.Config.Mu.Lock()

If possible, I would change the definition of posting.Config from

type Config struct {
...
Mu sync.Mutex
...
}

to

type Config struct {
...
sync.Mutex
...
}

so you can write this line as posting.Config.Lock().

Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I think after release we should go through and reduce boilerplate in these admin fns.

Same as with backup, we need to confirm if the old endpoints are being removed and then put up other PRs that do that and convert the tests over.

Reviewable status: 4 of 9 files reviewed, 6 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)


graphql/admin/admin.go, line 98 at r1 (raw file):

	}

	type ExportPayload {

(same comment holds here for backup endpoint)

How sure are we about these responses? If Dgraph always responds with just code and message, has done so for a while and we don't think we'll change that ... then it's probably ok to just return type Response for all of these.

The way you have done it is more flexible and might avoid changes in the future, but this is one spot where we'd save a bunch of types.


graphql/admin/admin.go, line 298 at r1 (raw file):

				})
		}).
		WithMutationResolver("export", func(m schema.Mutation) resolve.MutationResolver {

same as with backup, check that these can be called while the server is starting, otherwise move to addConnectedAdminResolvers


graphql/admin/admin.go, line 494 at r1 (raw file):

}

func writeResponse(m schema.Mutation, code, message string) []byte {

see note in backup.

Just write code, message and m.Name here. Aliases and selections are all handled generically now.


graphql/admin/config.go, line 57 at r1 (raw file):

	result map[string]interface{}) (*gql.GraphQuery, error) {

	return nil, nil

this is fine for now ... if we weren't under release pressure, we'd change some of this setup so you have to write less boilerplate.

Let's do a pass after release and make the works of all these admin bits one step nicer.

Copy link
Contributor Author

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I'll check with @manishrjain again if we are removing the old endpoints.

Reviewable status: 2 of 11 files reviewed, 6 unresolved discussions (waiting on @manishrjain, @martinmr, and @MichaelJCompton)


graphql/admin/admin.go, line 98 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

(same comment holds here for backup endpoint)

How sure are we about these responses? If Dgraph always responds with just code and message, has done so for a while and we don't think we'll change that ... then it's probably ok to just return type Response for all of these.

The way you have done it is more flexible and might avoid changes in the future, but this is one spot where we'd save a bunch of types.

Keeping it as it is, will do this change along with GRAPHQL-250 before the release.


graphql/admin/admin.go, line 298 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

same as with backup, check that these can be called while the server is starting, otherwise move to addConnectedAdminResolvers

Hmm, looking at the code I am not sure why we have two functions for doing all this as initServer is called right after newAdminResolverFactory so there isn't anything substantial that happens between the two. Maybe earlier we were doing a schema Alter, hence we had these differentiation?

Dgraph itself should return an error if its not ready to perform these operations, so we should be safe here.


graphql/admin/admin.go, line 494 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

see note in backup.

Just write code, message and m.Name here. Aliases and selections are all handled generically now.

I'll add a card to clean this up after the health endpoint PR is merged next week. Keeping it as it is for now.

https://dgraph.atlassian.net/browse/GRAPHQL-250


graphql/admin/config.go, line 57 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

this is fine for now ... if we weren't under release pressure, we'd change some of this setup so you have to write less boilerplate.

Let's do a pass after release and make the works of all these admin bits one step nicer.

Agree, there is too much boilerplate to conform to the interface that is defined here.
Created https://dgraph.atlassian.net/browse/GRAPHQL-249


worker/groups.go, line 1084 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I am assuming this is done to avoid hogging the CPU, right?

That is right


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

Previously, martinmr (Martin Martinez Rivera) wrote…

If possible, I would change the definition of posting.Config from

type Config struct {
...
Mu sync.Mutex
...
}

to

type Config struct {
...
sync.Mutex
...
}

so you can write this line as posting.Config.Lock().

done

@pawanrawal pawanrawal merged commit c39bd3f into master Feb 7, 2020
@pawanrawal pawanrawal deleted the pawanrawal/make-export-graphql-compatible branch February 7, 2020 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants