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

Remove subscriber reference from topic when deleted #1288

Merged
merged 20 commits into from
Apr 15, 2020

Conversation

tlaverdure
Copy link
Collaborator

@tlaverdure tlaverdure commented Apr 13, 2020

  • Added or updated tests
  • Added Docs for all relevant versions
  • Updated CHANGELOG.md

Changes

When subscribers are deleted their subscription key is not removed from the topic they are subscribed to. This should reduce the amount of calls to $this->cache->get() to check if the subscriber exists before broadcasting.

Breaking changes

N/A

When subscribers are deleted their subscription key is not removed from the topic they are subscribed to. This should reduce the amount of calls to `$this->cache->get()` to check if the subscriber exists before broadcasting.
When subscribers are deleted their subscription key is not removed from the topic they are subscribed to. This should reduce the amount of calls to `$this->cache->get()` to check if the subscriber exists before broadcasting.
@stayallive
Copy link
Collaborator

I have been thinking on how to solve this problem to and #1284 solved it a tiny bit, but not really... so when building a Redis optimized version I came up with wrapping the subscriber so we can store it with the topic it subscribed to.

https://gist.github.com/stayallive/9b9fe8556faf9c8ba6f09ca54610e40f#file-topicsubscriber-php-L7-L28

This way on deleting the subscriber we can remove the channel from the topic (in my case using srem to remove it from the Redis set).

https://gist.github.com/stayallive/9b9fe8556faf9c8ba6f09ca54610e40f#file-redissubscriptionstorage-php-L104-L111

This requires wrapping the topic with the subscriber when setting the subscriber info.

https://gist.github.com/stayallive/9b9fe8556faf9c8ba6f09ca54610e40f#file-redissubscriptionstorage-php-L90-L94

This might be a good direction to go in to prevent recreating the topic key because that just won't work since a subscription can have a custom topic encoder/decoder.

Note: Everything in that gist is experimental and cannot be used until #1286 & #1289 are merged and released otherwise you cannot swap out the subscriber storage class.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some things I think are wrong with this PR and I propose a discussion on how to implement a better way to implement this: #1288 (comment).

src/Subscriptions/StorageManager.php Outdated Show resolved Hide resolved
src/Subscriptions/StorageManager.php Outdated Show resolved Hide resolved
src/Subscriptions/StorageManager.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Apr 14, 2020

Can you merge the latest changes from master? I changed a few calls related to Cache::has().

@tlaverdure tlaverdure marked this pull request as ready for review April 14, 2020 14:11
@tlaverdure
Copy link
Collaborator Author

Thanks @stayallive, your suggestions were coming in realtime as I was on the same lines, haha.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last little nitpicky comments 😄

CHANGELOG.md Outdated Show resolved Hide resolved
src/Subscriptions/StorageManager.php Show resolved Hide resolved
@stayallive
Copy link
Collaborator

@tlaverdure perfect timing, thanks for keeping up with them and integrating them in your PR, this is starting to look real good 😎

tlaverdure and others added 2 commits April 14, 2020 10:40
Co-Authored-By: Alex Bouma <me@alexbouma.me>
Co-Authored-By: Alex Bouma <me@alexbouma.me>
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the StyleCI fixes left, but after that this looks pretty good to me! 👍

@spawnia
Copy link
Collaborator

spawnia commented Apr 14, 2020

The PR would not have worked as provided, serialization for $subscriber->topic was missing. I reworked the whole thing quite a bit and ensure we got it covered through tests. Sorry for blowing up this PR 🤯

@tlaverdure @stayallive If you could review the changes, that would be great. Great work so far.

@tlaverdure
Copy link
Collaborator Author

All good! Thanks for the additions, it makes things a lot clearer. Looks good to me.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little nitpick, but other than that this looks good 👍

@spawnia spawnia changed the title Remove Subscribers from Topics Remove subscriber reference from topic when deleted Apr 15, 2020
@spawnia spawnia merged commit c36b72f into nuwave:master Apr 15, 2020
@tlaverdure tlaverdure deleted the patch-2 branch May 21, 2020 14:42
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

Successfully merging this pull request may close these issues.

3 participants