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

Wondering why FINGERPRINTS_PREFIX SUBSCRIPTIONS_PREFIX keys don't have EXPIRE? #25

Open
gastonmorixe opened this issue Jun 29, 2022 · 1 comment

Comments

@gastonmorixe
Copy link

gastonmorixe commented Jun 29, 2022

We only apply EXPIRE to CHANNEL_PREFIX and SUBSCRIPTION_PREFIX. Would like to understand the reasons why we can't apply it to the other keys.

redis.multi do |pipeline|
pipeline.sadd(CHANNEL_PREFIX + channel_uniq_id, subscription_id)
pipeline.mapped_hmset(SUBSCRIPTION_PREFIX + subscription_id, data)
events.each do |event|
pipeline.zincrby(FINGERPRINTS_PREFIX + event.topic, 1, event.fingerprint)
pipeline.sadd(SUBSCRIPTIONS_PREFIX + event.fingerprint, subscription_id)
end
next unless config.subscription_expiration_seconds
pipeline.expire(CHANNEL_PREFIX + channel_uniq_id, config.subscription_expiration_seconds)
pipeline.expire(SUBSCRIPTION_PREFIX + subscription_id, config.subscription_expiration_seconds)
end
end

Thank you

@gastonmorixe gastonmorixe changed the title Wonder why FINGERPRINTS_PREFIX SUBSCRIPTIONS_PREFIX keys don't have EXPIRE Wondering why FINGERPRINTS_PREFIX SUBSCRIPTIONS_PREFIX keys don't have EXPIRE? Jun 29, 2022
@Envek
Copy link
Member

Envek commented Jul 5, 2022

TL;DR: I just not sure that it is safe to apply expiration to other data structures, that's why I haven't added expiration on them. But I'm open to suggestions.

There are two opposite directions on that data structures are traversed:

  1. On subscription or unsubscription, we create or delete subscription and channel for every client and register it for topics and fingerprints.
  2. On event trigger, we search by topics and fingerprints queries that should be evaluated, evaluate them and broadcast results by channels.

Current expiration logic covers only first direction to clean up data left after clients that didn't disconnect cleanly for some reason, and doesn't care about opposite direction.

The question is whether it is safe to set expiration on topic-fingerprints and fingerprint-subscriptions sets (and not to forget to bump them on every client subscriprions) without probability to get them expired too early and left some clients without updates?

Right now, if you're executing cleaning rake task graphql:anycable:clean regularly, it will detect and remove absent entries from these sets here and Redis will automatically remove sets that became empty.

However, I haven't thought about it much, so if you have any thoughts on how to prove safety or non-safety, please share!

But in general I'm open to adding expiration.

P.S> Please tell more about your current problem, why you're asking?

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

No branches or pull requests

2 participants