vektah / dataloaden

go generate based DataLoader
MIT License
530 stars 79 forks source link

Architecture using timeouts to call fetch? #14

Open BrendanBall opened 6 years ago

BrendanBall commented 6 years ago

I've so far been using graphql in node and have used the facebook dataloader quite a bit. I have some go services that I want to add a graphql api to. Gqlgen seems quite legit and I've read good things about it so far. However the way this dataloader works doesn't feel right. The facebook dataloader in js doesn't have such a weird implementation where you have to wait an arbitrary amount of time to resolve the data. Did you have trouble finding a solution in go that doesn't involve timeouts to know when to resolve the data? I would like to explore this a bit as it's currently preventing me from deciding to use this library. Can you maybe give some info on what you tried and how you decided that timeouts is the best solution? I think it would be good to include this in the readme, as I'm sure I'm not the only one that has concerns about this.

vektah commented 6 years ago

IIRC the facebook implementation batches together everything on the same "tick", golang doesn't have ticks. The timeout gives you the freedom to tweak how long you want to wait for a batch.

Now we could kind of simulate a tick in gqlgen right after starting all the concurrent resolvers (and maybe a gosched to make sure they have a chance to run). Given dataloaders are usually in context, this probably needs some smart context magic, and ideally can be done in a way that doesn't tie gqlgen directly to dataloaden and allows other libraries to receive that "resolvers issued" tick, if they want it.

In practice short timeouts (~1ms) work well enough and there are bigger fish to fry, but if someone wants to spec it out properly, and open to a PR that would be awesome :)

lubo commented 5 years ago

We're currently looking for a better implementation that doesn't depend on timeouts. @vektah Any specific ideas how to implement it? Short timeouts don't work well for us :/

CC @mskrip

vektah commented 5 years ago

related https://github.com/99designs/gqlgen/issues/518#issuecomment-458767743