-
Notifications
You must be signed in to change notification settings - Fork 37
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
ISSUE-457 - Improve performance for I/O bound list resolvers by using parallel execution #458
Conversation
Seems really cool @Maximilien-R |
Following our discussions on the #457 issue, here is a first draft aiming to fix it. Currently, I do fix only the list output coercion aspect. On the issue thread, I suggested to do something similar for output object coercion however this topic will be less straight forward. Indeed, there is the case for abstract object (Union & Interface) that should be discuss before going forward. Also, since on I think we could go on the object/abstract coercion customization on another PR. What I would like to do it to be able to really fine tuning it per object field instead of the parent field like its done for list. For instance, if we had the following SDL: type Author {
id: ID!
firstName: String!
lastName: String!
birthdate: Date!
}
type Book {
id: ID!
title: String!
authors: [Author!]
nbPages: Int!
resume: String!
opinions: [String!]
}
type Query {
books: [Book!]
} We could imagine that the As it's designed currently, we'll We could have something like: @Resolver("Query.books")
async def resolve_query_books(parent, args, ctx, info):
return Books.objects.all()
@Resolver("Book.authors", concurrently=True)
async def resolve_book_authors(parent, args, ctx, info):
return Authors.objects.filter(pk__in=parent["authors"]).all()
@Resolver("Book.opinions", concurrently=True)
async def resolve_book_authors(parent, args, ctx, info):
return Opinions.objects.filter(book_id=parent["id"]).all() The aim is to avoid to create async task for resolver that will only fetch data through a dumb (or the default) resolver which will only do a The difference with the list is that its the field resolver which will impact the parent resolving process. Tell me if I'm not clear 😆 |
I agree that it should be discussed in another PR, we'll keep this one for List only so it's clearer :) |
Looks good @Maximilien-R thank you very much I understand what you are saying about only doing concurrent resolution if there is a field that needs I/O (ie. the field has a non-dumb resolver), and agree. I suspect that applies to the list case too, not just objects - a list only really needs a concurrent resolver because one or more of the fields on each item needs concurrent resolution. So it seems broadly similar, just 1 step further removed (list_field -> item_in_list_field -> field_on_item_with_non_default_resolver) |
@Maximilien-R shall we merge ? |
f65955e
to
4a86953
Compare
@abusi, I guess we could if it's fine for you 👍 Note: if we want to do something similar with objects (according to my previous comment) we should probably change the parameter name to focus on the list aspect of |
Kudos, SonarCloud Quality Gate passed! |
Closes #457.