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.

Sign up for GitHub

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

Jump to bottom

WIP: shared subscriptions #437

Closed
wants to merge 9 commits into from
Closed

WIP: shared subscriptions #437

wants to merge 9 commits into from

Conversation

robertsLando
Copy link
Member

This is a work in progress for the feature. I would like to know your opinion about this idea and if this could be the right direction or not. @gnought @mcollina

#228

@github-actions
Copy link

Pull Request Test Coverage Report for Build 52228130978ea33f915ca07299741915108dd4a3-PR-437

  • -21 of 33 (36.36%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.01%) to 96.708%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/handlers/validations.js 1 4 25.0%
lib/handlers/subscribe.js 5 9 55.56%
lib/group.js 4 18 22.22%
Totals Coverage Status
Change from base Build 9c90d2ef6ba7f4796258cbe16b4d6e0d6ea38e96: -3.01%
Covered Lines: 774
Relevant Lines: 795
💛 - Coveralls

@github-actions
Copy link

Pull Request Test Coverage Report for Build 52228130978ea33f915ca07299741915108dd4a3-PR-437

  • 12 of 33 (36.36%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.01%) to 96.708%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/handlers/validations.js 1 4 25.0%
lib/handlers/subscribe.js 5 9 55.56%
lib/group.js 4 18 22.22%
Totals Coverage Status
Change from base Build 9c90d2ef6ba7f4796258cbe16b4d6e0d6ea38e96: -3.01%
Covered Lines: 774
Relevant Lines: 795
💛 - Coveralls

mcollina
mcollina requested changes Feb 20, 2020
View reviewed changes
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

This will not work in the multi-process case as the subscription group can be connected to multiple hosts.

@robertsLando
Copy link
Member Author

@mcollina So what should be the way? Suggestions?

@mcollina
Copy link
Collaborator

It needs to be architected/investigated, I’m basically stating requirements. I currently do not have time for doing so in the near future.

A practical way is to have a “plugin” built on top of Redis Streams. This might have significant limitation related to how #/+ work.

A more complex solution is to publish the “active” shared subscription for each peer in the broker metadata, and have the message being routed to a different broker if the current one does not have a an active shared subscription for that topic.

@robertsLando
Copy link
Member Author

Could my actual implementation work in a single-instance model at least?

About your answer I think the best way would be to add shared subscriptions to aedes-persistence or it becomes hard to handle. I was thinking about a solution using $SYS topics but I think it becomes complex and has very bad efficiency

@gnought
Copy link
Collaborator

gnought commented Feb 21, 2020

good try.
imo. aedes is a multi process model, we need to make it flexile rather than implemented in single-instance.

@robertsLando
Copy link
Member Author

robertsLando commented Feb 21, 2020 •
edited
Loading

@gnought Could we speak about a possible solution for this? I think a possible soulutions could be:

Once we receive a shared subscription we add the subscription to persistence with a lastUpdate date set to null. Before each delivery the client will check in the persistence for the shared subscription to that topic with the lower lastUpdate and if the subscription returned has his clientId he delivers the packet and update his subscription lastUpdate, if not it will skip the delivery.

In poor words we need to edit aedes-persistence:

  1. addSubscriptions Method should also add a lastUpdate for shared subscriptions (this could be skipped as we can handle field that doesn't have this)
  2. Create a method updateSharedSubscription that updates lastUpdate date of a shared subscription
  3. Create a method nextSharedSubscription that returns the next shared subscription that should get the update.

In aedes we just need to add a deliverShared to client.js that will be the function to bind when receving a shared subscription. The subscription must be made with the topic without the $share and groupid prefix

Should this work? Thoughts?

@gnought
Copy link
Collaborator

gnought commented Feb 21, 2020

There may be more work to do before that, and make the whole picture be clear. Would you mind to extract out interfaces that other aedes persistence should implement to, it is nicer if we have a typing in aedes-persistence.

@robertsLando
Copy link
Member Author

@gnought Check moscajs/aedes-persistence#48 Than I will add new methods

@robertsLando
Copy link
Member Author

@gnought I close this and start working on the new one

@robertsLando robertsLando deleted the sharedsubscriptions branch February 24, 2020 12:51
@robertsLando robertsLando mentioned this pull request Feb 24, 2020
Draft
@abhargav7
Copy link

@robertsLando does closing means you are creating a PR from new branch sooner, or case is closed :)
Thanks for supporting this.

@coveralls
Copy link

Pull Request Test Coverage Report for Build d86511e6b44d254d2d57d59b683a14b6bba17589-PR-437

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 18 of 59 (30.51%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-5.6%) to 94.123%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/handlers/unsubscribe.js 6 7 85.71%
lib/handlers/validations.js 1 4 25.0%
lib/handlers/subscribe.js 5 10 50.0%
lib/group.js 4 36 11.11%
Totals Coverage Status
Change from base Build 9c90d2ef6ba7f4796258cbe16b4d6e0d6ea38e96: -5.6%
Covered Lines: 775
Relevant Lines: 816
💛 - Coveralls

@coveralls
Copy link

coveralls commented Nov 23, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 52228130978ea33f915ca07299741915108dd4a3-PR-437

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 21 of 42 (50.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.0%) to 96.708%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/handlers/validations.js 1 4 25.0%
lib/handlers/subscribe.js 6 10 60.0%
lib/group.js 4 18 22.22%
Totals Coverage Status
Change from base Build 9c90d2ef6ba7f4796258cbe16b4d6e0d6ea38e96: -3.0%
Covered Lines: 774
Relevant Lines: 795
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina Awaiting requested review from mcollina

@gnought gnought Awaiting requested review from gnought

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@robertsLando @mcollina @gnought @abhargav7 @coveralls