-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
Only store history when there are already subscribers #50
Comments
about the Edge Case: Can you treat channel is active when at least one user online(SUB) in TTL of
I think this is somehow solve the Edge Case. If one channel has no subscription in TTL time, then it is inactive, otherwise it is active. For those channel with no subscription in a long time, it always inactive, no memory cost. And to implement this is also simple, just one field |
@roytan883 looks like pretty reasonable and useful suggestion. I doubt though that it's possible to implement in a way you've written - 2 reasons against:
I have another idea - can't decide now is it pretty or ugly:) Or will it even work?:) Anyway: when last user in channel disconnects from node we usually unsubscribe this node from channel in Redis. What if we won't unsubscribe node until history TTL time expires. This way we will have subscription on Redis channel and message will be saved into history. This behaviour must be optional. And it's not simple to implement as I can see. |
@FZambia For this scenario:
should work, mark this channel is active, because the channel has subscriber. So the active conditions should be any one of the three:
Also I think the first rule can optimize the
|
Sorry, I am not sure that I understood you right - especially part about any of 3 points (I only understand point 1). We already
I understand what you want here and understand how we can solve this (my suggestion above and your suggestion from point 1). Maybe you could write more accurately what you mean in other parts of your previous comment. |
I think @roytan883 means: if every channel has a Redis key containing a "last activity" timestamp then there is no need to use the current "try to publish and see if anyone gets it, try to save if history exists" technique. But I'm not sure it's an optimisation - currently we can do it all in one round trip with a redis script anyway so cost is extremely low, the only real saving would be that if you did check first you could possibly avoid serialisation etc if message entirely but then the active case would require two round trips, one to check if publish needs and one to do it which is almost certainly much worse performing than now. I think we should forget about this suggestion being an optimisation - it's either not or is likely insignificant - and just consider it on the basis of less surprising behaviour. One alternative that occurs to me would be that we could publish "null" messages to a channel on client subscribe or unsubscribe. They could consume almost no memory and would get the desired behaviour without changing either the publish/history logic or adding new keys into redis. @FZambia how easy would it be to introduce a null message that is just ignored by engine code that handles subscribed messages but is enough to keep a history channel alive for the activity TTL? Seems easy and less hacks than alternatives. Especially if we only publish them in the case that the channel IS inactive at the time (that might be harder though). |
@banks it's an interesting idea - I have not thought about this from a side that we can just create history key with TTL or prolong existing on TTL. Let me clarify - you mean "null" messages that will be published in Redis and not actually added into history and won't be published into channel right? The goal is just to Maybe we should do this as separate routine without mixing publish logic with such history touch. It's an implementation detail of course but could be semantically cleaner and not exposed to public API anyway. Also need to think about mass resubscription case after reconnect to Redis and how this will affect existing metrics. |
I was thinking actually publish a "messsage" which is just a 0x0 Byte or something so it uses same code paths etc and just drop messages like that when reading history back (before delivering to client).
But yeah could probably be done with a "touch" of the history key without inserting message if that's cleaner.
Either way should be transparent to clients.
… On 31 Jan 2017, at 08:11, Alexandr Emelin ***@***.***> wrote:
@banks it's an interesting idea - I have not thought about this from a side that we can just create history key with TTL or prolong existing on TTL.
Let me clarify - you mean "null" messages that will be published in Redis and not actually added into history and won't be published into channel right? The goal is just to touch history.
Maybe we should do this as separate routine without mixing publish logic with such history touch. It's an implementation detail of course but could be semantically cleaner and not exposed to public API anyway. Also need to think about mass resubscription case after reconnect to Redis and how this will affect existing metrics.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Got it, thanks! Will try to make proof of concept, maybe some details will arise in process. |
@roytan883 @banks I ended up with pull request #148 - please have a look! |
This is an RFC for a new (optional) optimisation that is crucial for us.
I'm going to work on implementing this today on a fork and will hopefully have a PR, but wanted to document the rationale and design first.
Problem
Right now, if history/recovery is enabled, Centrifugo will store up to
history_size
messages for every channel, published to in the lasthistory_lifetime
seconds.This makes designs with high degree of fan-out impractically expensive.
In my case I have about 5k messages a second being published (after fan-out) where some messages might be published into tens of thousand of channels - one for each "watcher". Even with no subscribers active and modest history settings, that means I need tens or hundreds of GB of Redis memory which is all doing nothing - never read by anyone.
In vast majority of cases, a user who is watched by tens of thousands of people will likely only have a very small fraction of them currently online and subscribed to centrifugo; most users with few watchers will have zero watchers online most of the time. I expect this optimisation to reduce redis memory requirements (and operations per second) by several orders of magnitude, with almost no change to app behaviour (see
Edge Case
section).Solution Overview
I propose an optimisation that will ensure we simply don't store history when it is not needed by "active" subscribers. "Active" means: online now, or within
history_lifetime
seconds (and so will possibly reconnect, expecting recovery).The logic below is almost transparent to the application - it will behave identically to current logic, but will result in only using redis memory for data that is actually delivered.
At a high level it's simple:
engine.publish()
actually delivered to anyonehistory_lifetime
)Implementation
This should be relatively simple. If I were designing from scratch I'd be tempted to not have separate
publish
andaddHistory
methods in theengine
interface as they are fairly closely linked. Indeed if you made that change it would be possible to implement this entire request in thepublish
method and potentially with a lua script inside redis entirely.But in interest of minimising changes to core centrifugo code I propose we implement it with these minimal changes:
Engine.publish
signature topublish(chID ChannelID, message []byte) (bool, error)
where the bool return value istrue
when we delivered to at least 1 active subscriber andfalse
otherwise.addHistory
's options struct to include a new option:OnlySaveIfActive bool
. If this is set totrue
thenaddHistory
should only add the new event if the history key already exists.Engine Compatibility
publish
are efficient to implement in memory engine as you have state about subscribersPUBLISH
returns the number of active subscriptions that were delivered to (http://redis.io/commands/publish)true
for every call, and you end up with exactly the same behaviour as now - suboptimal but totally correct.history_lifetime
. Clients end up with identical deliverability guarantees - indeed they couldn't tell the difference with and without this.Edge Case (resolved with #148 )
I've said this is //almost// completely transparent to the client which is true. There is one case in which it is not which I regard as an edge case, and acceptable given centrifugos "best effort" general design.
Given the proposed optimisation, consider the following:
foo
A
is published intofoo
history_lifetime
withrecover
option. But they don't getA
because it was not saved.Personally I don't consider this a big problem. However given that it is a change, we could consider making this optimisation configurable in case others find it unacceptable.
Self-subscriptions
The one other case that doesn't affect me but should be pointed out is that if you are using the Websocket API to allow publishing (i.e.
publish: true
in config) then these messages will always be saved due to the client API design - a client can't publish to a socket unless they have a subscription to it which meanspublish()
will always see at least one active subscription.This is no worse that current behaviour and is probably expected/desirable anyway so I don't think it's an argument against the optimisation.
The text was updated successfully, but these errors were encountered: