-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat: have flightGroups take some time to allow reuse #286
base: v8
Are you sure you want to change the base?
Conversation
@@ -109,6 +110,7 @@ func (r *serverSideEnvStreamRepository) Replay(channel, id string) chan eventsou | |||
// getReplayEvent will return a ServerSidePutEvent with all the data needed for a Replay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only called when we have a new or reconnecting client.
So some small delay here should be acceptable.
e6a11e1
to
d22388d
Compare
Hi @moshegood, I like the idea here - trading increased latency for decreased memory usage. In your use-case, what would you actually use as the configured One downside I'm seeing is that the latency would be introduced even if no one else is being served. A dynamic approach might calculate the incoming RPS and use that to increase the latency, but that might be too complicated. |
I've been testing with 2 seconds. |
2976a5e
to
52ff6a5
Compare
Would it be better if we only delay when there was another request completed very recently? |
I think so, and I'm wondering if we can use something like token bucket to solve this. For example:
|
Changed things around so that a single new client connection is responded to immediately. |
2ea42ef
to
f517e61
Compare
aaf06f9
to
2c8e623
Compare
Any updates required on this PR? |
Hi @moshegood , pardon the delay on my end. We are still discussing how to approach this internally. Mainly it seems that we might have outgrown the usage of the flightgroup and need a more sophisticated system. |
Max memory usage on two clusters with/without this change.
The lower line is the one with the change.
Note also, the high memory usage servers were actively shedding load and rejecting connections to stay alive.
Other Approaches
The main other way to do this would be to cache the data from the
MakeServerSidePutEvent(...)
call ingetReplayEvent()
That would require cache-invalidation, which would require the data stores to have some version id to enable the ld-relay to know that the underlying information has changed.
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.