[go: up one dir, main page]

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

graphman: create GraphQL API to execute commands #5554

Merged
merged 10 commits into from
Oct 1, 2024

Conversation

isum
Copy link
Contributor
@isum isum commented Jul 18, 2024

This PR creates a new graphman GraphQL API with initial support for deployment info, pause, resume and restart commands. It also provides the building blocks needed to support more complex graphman commands.

Part of #3729

Note

The GraphQL API will be extended incrementally, so there will be a transition period when graphman CLI modules will have weird type conversions and potentially duplicate code. That will go away when all commands are extracted into the core crate.

Todos

  • Integrate extracted commands back into graphman CLI
  • Add tests to the API using a real database connection
  • Add documentation for the new graphman API
  • Add support for long running graphman commands
  • Fix metrics initialization error
  • Extend the API with the remaining commands Will be in the scope of another PR
  • Add documentation on how to extend the GraphQL API with new commands

@isum isum self-assigned this Jul 18, 2024
@fordN fordN linked an issue Jul 18, 2024 that may be closed by this pull request
@fordN fordN requested a review from encalypto July 18, 2024 15:44
@isum isum force-pushed the ion/expose-graphman-commands-through-graphql branch 2 times, most recently from 0bb243e to a765043 Compare July 22, 2024 18:34
@isum isum force-pushed the ion/expose-graphman-commands-through-graphql branch from 9785e67 to 6fe37ae Compare July 30, 2024 15:26
Copy link
Contributor
@encalypto encalypto left a comment

Choose a reason for hiding this comment

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

Generally LGTM! 👍

It might be worth getting a second opinion for the command creation and extension layering APIs

@fordN fordN requested a review from lutter August 8, 2024 15:51
Copy link
Collaborator
@lutter lutter left a comment

Choose a reason for hiding this comment

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

The code is very well written, and very clean. Nice! I am still not convinced we need all the machinery around layering and that background execution could be simplified - I'll leave a longer comment about that and some other things I noticed.

store/postgres/src/graphman_store/mod.rs Outdated Show resolved Hide resolved
core/graphman/src/commands/deployment/pause.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
core/graphman_extensions/src/handle_broken_executions.rs Outdated Show resolved Hide resolved
docs/graphman-graphql-api.md Outdated Show resolved Hide resolved
graph/src/env/mod.rs Outdated Show resolved Hide resolved
node/src/bin/manager.rs Outdated Show resolved Hide resolved
@lutter
Copy link
Collaborator
lutter commented Aug 21, 2024

I noticed a few things in the GraphQL schema (which is well documented):

  • the schema has a QueryRoot and MutationRoot. IIRC, at least for queries, that must be called Query
  • for info to filter by namespace, you have to say info(deployment: { namespace: { namespace: "sgd86"}} ), i.e., mention namespace twice. Couldn't that just be info(deployment: { namespace: "sgd86"}) ? Similarly, when looking for a subgraph by name, that could be shortened a bit, too.
  • very minor, but I don't think the dpeloyment info should expose the id, that's an implementation detail.

@lutter
Copy link
Collaborator
lutter commented Aug 22, 2024

For the general approach how to expose the same business logic in the CLI and API, if you have a function biz that implements the business logic (I'll write as if this was a thunk, but it should probably be a trait), you want to do one of two things:

Run biz in the CLI

  • parameters come from the command line (clap) and possibly some general setup like the store
  • the function biz() should run synchronously, even if the command might take a long time
  • output should go straight to the terminal (ideally, there's access to stdout and stderr, though I don't think we use that today)
  • success/failure is indicated by the exit status
  • hitting ^C should immediately abort the command
  • being conscious about how much users have to type to run a command is a consideration

Run biz in the API

  • parameters come from the GraphQL query and might use existing setup like the store
  • the function biz() should run as part of a larger setup where a long-running biz() runs in the background, i.e., not synchronously with the GraphQL request.
  • output should be logged, though what should be logged might be different from what the CLI prints on the terminal
  • success/failure of the request just indicates whether the task to run biz() has been accepted

For the API case, it seems to me that it would be enough to have something that wraps biz() in the additional logic that background execution requires, something like

fn run(biz: Biz, ctx: Context) -> Result <_,_> {
  // The main thing that cmd_ctx allows is to change the status (or progress) of the command
  // for very long-running things
  // This also glosses over preventing duplicate execution, and how to tell generically whether two commands are the same
  // but that would happen here, too
  let cmd_ctx = add_row_to_graphman_command_executions(..)
  let res = biz.execute(ctx, cmd_ctx);
  cmd_ctx.command_finished();
  res
}

This might need to be split up a bit to make the right pieces async, but it could be fairly linear code with little need for extension points.

@isum isum force-pushed the ion/expose-graphman-commands-through-graphql branch 4 times, most recently from 553cfcd to 341fb43 Compare September 18, 2024 14:42
@isum isum requested a review from lutter September 18, 2024 15:15
Copy link
Collaborator
@lutter lutter left a comment

Choose a reason for hiding this comment

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

Very nice! I really like this and how it is put together.

core/graphman_store/src/lib.rs Show resolved Hide resolved
core/graphman/src/execution_tracker.rs Outdated Show resolved Hide resolved
docs/graphman-graphql-api.md Outdated Show resolved Hide resolved
docs/graphman-graphql-api.md Outdated Show resolved Hide resolved
@isum isum force-pushed the ion/expose-graphman-commands-through-graphql branch from 1a39fe3 to b4c7c24 Compare October 1, 2024 13:01
@isum isum merged commit 2509212 into master Oct 1, 2024
7 checks passed
@isum isum deleted the ion/expose-graphman-commands-through-graphql branch October 1, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose graphman commands through graph-node's admin GraphQL interface
3 participants