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] instigation state by id #22543

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

alangenfeld
Copy link
Member

@alangenfeld alangenfeld commented Jun 13, 2024

  • add CompoundID to wrap external origin + selector id components
  • change the Schedule Sensor and Repository id fields to return serialized CompoundIDs
  • update the instigationStateOrError to be able to fetch by id
  • add instigationStatesOrError and support fetching all instigation states by their containing repository ID

How I Tested These Changes

Added tests

Copy link
Member Author

alangenfeld commented Jun 13, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alangenfeld and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Jun 13, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-605yi21au-elementl.vercel.app
https://al-06-13--graphql-instigation-state-by-id.core-storybook.dagster-docs.io

Built with commit e1a9fa7.
This pull request is being automatically deployed with vercel-action

@alangenfeld alangenfeld requested a review from salazarm June 13, 2024 21:27
@@ -281,13 +288,22 @@ class Meta:

instigationStateOrError = graphene.Field(
graphene.NonNull(GrapheneInstigationStateOrError),
instigationSelector=graphene.NonNull(GrapheneInstigationSelector),
instigationSelector=graphene.Argument(GrapheneInstigationSelector),
id=graphene.Argument(graphene.String),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this take an array of Ids instead so that I can use our batching abstraction :)

Copy link
Member Author

Choose a reason for hiding this comment

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

i can on the "states" resolver -I need to add the database look impl for many by id so will be a stacked PR. Thought maybe converting "SingleXQuery` in place would be the move but I can see wanting to move to batching while we are changing things

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind keeping individual queries. It's likely faster since theres more parallelization, but also slight more taxing on the server. gcpu vs ux latency trade off

@alangenfeld alangenfeld force-pushed the al/06-13-_graphql_instigation_state_by_id branch 3 times, most recently from 3c8fe6c to 3677301 Compare June 14, 2024 17:26
@alangenfeld alangenfeld requested review from gibsondan and prha June 14, 2024 17:28
@alangenfeld alangenfeld force-pushed the al/06-13-_graphql_instigation_state_by_id branch from 3677301 to 0b0fc9c Compare June 14, 2024 17:37


@dagster_model
class CompoundID:
Copy link
Member

Choose a reason for hiding this comment

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

this name feels a little generic considering it's specific to instigators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Repository uses it too which is why i dropped instigator framing
open to other names tho

Copy link
Member

Choose a reason for hiding this comment

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

CompoundExternalId
OriginAndSelectorId

meh, not user facing / changeable in the future since not persisted

@alangenfeld alangenfeld force-pushed the al/06-13-_graphql_instigation_state_by_id branch from 0b0fc9c to 1d08104 Compare June 21, 2024 22:04


@dagster_model
class CompoundID:
Copy link
Member

Choose a reason for hiding this comment

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

CompoundExternalId
OriginAndSelectorId

meh, not user facing / changeable in the future since not persisted

@alangenfeld alangenfeld force-pushed the al/06-13-_graphql_instigation_state_by_id branch from 1d08104 to e1a9fa7 Compare July 1, 2024 18:17
@alangenfeld alangenfeld merged commit 18ed7c8 into master Jul 1, 2024
2 checks passed
@alangenfeld alangenfeld deleted the al/06-13-_graphql_instigation_state_by_id branch July 1, 2024 18:55
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.

4 participants