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

Improve logging of old clientID in Container: reduce impact on perf #3675

Closed
vladsud opened this issue Sep 18, 2020 · 0 comments
Closed

Improve logging of old clientID in Container: reduce impact on perf #3675

vladsud opened this issue Sep 18, 2020 · 0 comments
Assignees
Labels
bug Something isn't working perf

Comments

@vladsud
Copy link
Contributor

vladsud commented Sep 18, 2020

This is follow up to #3613
See more detailed discussion in #3656

This kind of logging can be done more elegantly.
I'd not track 100 clients - we can avoid it via following 2 checks:

  1. Any incoming message should be from clientID that is in quorum, from "active" client.
  2. Whenever we reconnect and transition to "connected" state, we mark our previous client as "dead" (or not "active"), if it's still in quorum. This can happen if client believes it lost connection, but server did not had a chance to react (or did not notice loss of connection) and might time-out later.

The reason I prefer this approach over one in main (above mentioned PR) is that we do not need to allocate a bunch of memory, and we also do not need to scan that much memory on any op. That's a bunch of memory to scan on every op!
While quorum can have a lot of members, it is a hash map (under the cover), so checking client is relatively cheap, and does not use more memory.

@vladsud vladsud added the bug Something isn't working label Sep 18, 2020
@ghost ghost added the triage label Sep 18, 2020
@vladsud vladsud self-assigned this Sep 18, 2020
@vladsud vladsud added this to the September 2020 milestone Sep 18, 2020
@ghost ghost added triage and removed triage labels Sep 18, 2020
@tylerbutler tylerbutler added perf and removed triage labels Sep 21, 2020
@ghost ghost added the triage label Sep 21, 2020
heliocliu added a commit that referenced this issue Sep 30, 2020
…d check perf (#3704)

#3675
Change the old/expired clientId check to use the quorum instead of by keeping a list of old clientIds. This works by adding a shouldHaveLeft field to the ISequencedClient in the quorum, where a reconnecting client will set its old clientId shouldHaveLeft to true (if it still exists in the client). Then when we process a message, instead of checking against a list of old clientIds, we check if the clientId exists in the quorum (messages should only be coming from clients in the quorum) and if the clientId has been marked should have left. An inactive clientId in this situation would suggest a message from an old connection of the same client (which shouldn't happen).
@vladsud vladsud removed their assignment Oct 1, 2020
heliocliu added a commit to heliocliu/FluidFramework-1 that referenced this issue Oct 1, 2020
…d check perf (microsoft#3704)

microsoft#3675
Change the old/expired clientId check to use the quorum instead of by keeping a list of old clientIds. This works by adding a shouldHaveLeft field to the ISequencedClient in the quorum, where a reconnecting client will set its old clientId shouldHaveLeft to true (if it still exists in the client). Then when we process a message, instead of checking against a list of old clientIds, we check if the clientId exists in the quorum (messages should only be coming from clients in the quorum) and if the clientId has been marked should have left. An inactive clientId in this situation would suggest a message from an old connection of the same client (which shouldn't happen).
heliocliu added a commit that referenced this issue Oct 1, 2020
…d check perf (#3704) (#3801)

#3675
Change the old/expired clientId check to use the quorum instead of by keeping a list of old clientIds. This works by adding a shouldHaveLeft field to the ISequencedClient in the quorum, where a reconnecting client will set its old clientId shouldHaveLeft to true (if it still exists in the client). Then when we process a message, instead of checking against a list of old clientIds, we check if the clientId exists in the quorum (messages should only be coming from clients in the quorum) and if the clientId has been marked should have left. An inactive clientId in this situation would suggest a message from an old connection of the same client (which shouldn't happen).
This was referenced Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working perf
Projects
None yet
Development

No branches or pull requests

3 participants