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

server settings for replication #547

Closed
wants to merge 9 commits into from
Closed

server settings for replication #547

wants to merge 9 commits into from

Conversation

kickster97
Copy link
Member

@kickster97 kickster97 commented Jul 12, 2023 •
edited

WHAT is this pull request doing?

Adds properties min_followers and max_lag to the Config class and the values are parsed from lavinmq.ini
The values are then conditioned in vhost.cr via the replication servers new wait_for_max_lag. You cannot publish to a queue if you have less followers connected than min_followers or if min_followers amount of followers have more lag than max_lag.

Fixes #674

HOW can this pull request be tested?

Start LavinMQ and configure the values in your config file, connect too few followers and try to publish.

@github-actions
Copy link

github-actions bot commented Jul 12, 2023 •
edited

Main benchmark
'Average publish rate: 395624.7 msgs/s'
'Average consume rate: 395963.5 msgs/s'

PR benchmark
'Average publish rate: 403315.6 msgs/s'
'Average consume rate: 401376.7 msgs/s'

Keep in mind, these numbers are not representative of LavinMQ's peak performance.
It is rather an indication of how the changes of this pull request affects the performance of the main branch.

carlhoerberg
carlhoerberg requested changes Jul 20, 2023
View reviewed changes
src/lavinmq/replication/server.cr Outdated Show resolved Hide resolved
src/lavinmq/replication/server.cr Outdated Show resolved Hide resolved
src/lavinmq/vhost.cr Outdated Show resolved Hide resolved
@kickster97 kickster97 marked this pull request as ready for review August 18, 2023 09:53
carlhoerberg
carlhoerberg requested changes Aug 22, 2023
View reviewed changes
src/lavinmq/vhost.cr Outdated Show resolved Hide resolved
@kickster97 kickster97 marked this pull request as draft September 5, 2023 14:32
@kickster97 kickster97 force-pushed the replication_settings branch 2 times, most recently from 6ff9af3 to 19a91b9 Compare September 11, 2023 12:08
@kickster97
Copy link
Member Author

kickster97 commented Sep 13, 2023 •
edited

@viktorerlingsson this is the solution i worked on with @carlhoerberg yesterday.

I added the wait_for_followers channel, which works if i take it slow with the followers, and don't have an agressive max_lag limit (around 10000b)
but gets into a bad state if the followers connect and disconnect too quickly when we have an agressive max_lag limit (like 100b)

@kickster97 kickster97 force-pushed the replication_settings branch 3 times, most recently from f70dfd6 to 65b0c7d Compare September 28, 2023 07:11
@kickster97 kickster97 marked this pull request as ready for review September 28, 2023 07:12
spuun
spuun reviewed Sep 28, 2023
View reviewed changes
src/lavinmq/vhost.cr Outdated Show resolved Hide resolved
spuun
spuun reviewed Nov 8, 2023
View reviewed changes
src/lavinmq/replication/server.cr Outdated Show resolved Hide resolved
src/lavinmq/replication/server.cr Outdated Show resolved Hide resolved
@kickster97 kickster97 requested a review from spuun November 9, 2023 14:42
spuun
spuun approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@spuun spuun left a comment

Choose a reason for hiding this comment

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

👍

@kickster97 kickster97 marked this pull request as draft January 8, 2024 14:13
@kickster97 kickster97 force-pushed the replication_settings branch 2 times, most recently from 7df8592 to 8136d05 Compare March 26, 2024 13:22
@spuun spuun mentioned this pull request Apr 8, 2024
Merged
@kickster97 kickster97 force-pushed the replication_settings branch 2 times, most recently from 9576048 to 14ec15e Compare April 26, 2024 12:56
@kickster97 kickster97 force-pushed the replication_settings branch 2 times, most recently from 0d6dacd to b672109 Compare May 7, 2024 08:59
@kickster97 kickster97 marked this pull request as ready for review May 20, 2024 08:01
kickster97
kickster97 commented May 20, 2024
View reviewed changes
spec/replication_spec.cr Outdated
when done.receive
fail "Should not receive message"
when timeout(0.1.seconds)
#ugly hack to release replicator from waiting for lag
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to work around spec_helpsers after_each() that calls server.stop here.. because server.stop only calls replicator.clear, which will not finish if we are waiting for lag.

@kickster97 kickster97 closed this May 21, 2024
@kickster97 kickster97 deleted the replication_settings branch May 31, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spuun spuun spuun approved these changes

@carlhoerberg carlhoerberg Awaiting requested review from carlhoerberg carlhoerberg is a code owner

@viktorerlingsson viktorerlingsson Awaiting requested review from viktorerlingsson viktorerlingsson is a code owner

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

Successfully merging this pull request may close these issues.

Minimum followers and Max Lag setting for replication
3 participants
@kickster97 @carlhoerberg @spuun