-
Notifications
You must be signed in to change notification settings - Fork 527
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
Adjust how subscription IDs are generated #654
Adjust how subscription IDs are generated #654
Conversation
@darrenclark Hi 👋
I'm wondering if you could simply rely on the |
@arkgil It will. There's two things happening here, there's the topic id which is used to figure out what events should fire your subscription, and then there's a "document id" concept which is used to de-duplicate documents. Document deduplification is important because it wildly improves the scale at which subscriptions can operate. If you have 1000 people all viewing the front page of your news site for exampled, you'll have 1000 copies of the same subscription document all submitted. By deduplicating, we can run just 1 of those and then publish the results to everyone. |
@benwilson512 right, I am aware of it, I wasn’t really clear with my comment. What I meant is, with the approach presented in this PR Subscriptions attached to the same topic are duplicated by default, and context_id needs to be explicitly set if you want them deduplicated. I was wondering whether having them deduplicated given the same topic would make sense. Or I’m just talking nonsense 😄 Anyway, thanks for clarification 🙂 |
@arkadiyk Imagine you have 10 documents submitted by 10 different users, each with the user set in the context. If you dedup only on the bodies, then any data loading that happens which takes into account the current user becomes horribly broken. Only the first document is run, which loads stuff that may be exclusive to just that user, and then it gets broadcasted to everyone. |
Regarding the status of this PR: Been busy with other things, but hoping to loop back to this in the next week or two |
Subscription IDs are now derived via a `context_id` option returned by a subscriptions `config` function, combined with the GraphQL document and any provided variables. This gives library users more control over which subscription queries get merged. If `context_id` is not provided, a unique integer is generated instead (to ensure we don't merge any queries by default) Additionally, if one wants to override the default behaviour of combining the GraphQL document & provided variables to determine the `document_id`, they can return a `document_id` from the subscription's `config` function
9ed45ee
to
f1183e6
Compare
@benwilson512 I (finally) got this updated, it's ready for review now. I did not document (outside of the tests for it) the @arkgil If you get a chance, can you let me know if this addresses #675 |
@darrenclark This is really fantastic, thank you! |
First pass at the changes described in #652
If things look good, I'll go update all documentation, fix conflicts, etc.
I also need to add the blueprint to the resolution struct (#652 (comment))
Subscription IDs are now derived via a
context_id
option returned by a subscriptionsconfig
function, combined with the GraphQL document and any provided variables.This gives library users more control over which subscription queries get merged.
If
context_id
is not provided, a unique integer is generated instead (to ensure we don't merge any queries by default).:erlang.term_to_binary
+ SHA256 hash dance on thecontext
by default instead. It should be safe to merge subscriptions whosecontext
is the same.. Thoughts?Additionally, if one wants to override the default behaviour of combining the GraphQL document & provided variables to determine the
document_id
, they can return adocument_id
from the subscription'sconfig
function