[WPB-10308] Use RabbitMQ queues for notifications#4272
Merged
mdimjasevic merged 101 commits intodevelopfrom Nov 12, 2024
Merged
[WPB-10308] Use RabbitMQ queues for notifications#4272mdimjasevic merged 101 commits intodevelopfrom
mdimjasevic merged 101 commits intodevelopfrom
Conversation
echoes-hq
bot
added
the
echoes/initiative: scale
zebot
added
the
ok-to-test
battermann
changed the title
akshaymankar
force-pushed
the
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
branch
3 times, most recently
from
842158f to
2974f24
Compare
fisx
force-pushed
the
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
branch
from
3321d48 to
861ee10
Compare
akshaymankar
reviewed
Oct 8, 2024
services/cannon/src/Cannon/Types.hs
Outdated
Comment on lines
122
to
123
Member
There was a problem hiding this comment.
There is a limit on number of channels we can open per connection, so this solution will likely break because of that. I think it might be best to go back to separate connections per client for now because of two reasons:
- I think I misjudged that openning ~20k connections will break rabbitmq, perhaps we just need to run many nodes.
- For this we should probably implement some smart connection pooling, which would take some time and look very different from this, so perhaps we can split this work up like that.
akshaymankar
force-pushed
the
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
branch
2 times, most recently
from
9c4421e to
0fc2e4e
Compare
2 tasks
akshaymankar
added a commit
that referenced
this pull request
Oct 10, 2024
It has been set to `true` for a few years now and seems to work well. It is being removed to reduce work for RabbitMq based notifications. Related: #4272 Also: https://wearezeta.atlassian.net/browse/WPB-10308
akshaymankar
added a commit
that referenced
this pull request
Oct 10, 2024
It has been set to `true` for a few years now and seems to work well. It is being removed to reduce work for RabbitMq based notifications. Related: #4272 Also: https://wearezeta.atlassian.net/browse/WPB-10308
akshaymankar
force-pushed
the
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
branch
from
3a2c797 to
1f0792d
Compare
akshaymankar
force-pushed
the
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
branch
from
b9355f2 to
c87d7b0
Compare
| @@ -1 +1 @@ | |||
| Notifications are now also send via RabbitMQ therefore RabbitMQ is now a required configuration in brig. | |||
| Notifications are now also sent via RabbitMQ therefore RabbitMQ is now a required configuration in brig. | |||
Member
There was a problem hiding this comment.
This is not true, brig shouldn't require rabbitmq unless federation is enabled.
Contributor
There was a problem hiding this comment.
I've updated the change log to fix the statement.
mdimjasevic
approved these changes
Oct 22, 2024
Comment on lines
+274
to
+275
| unless isAckChanEmpty $ do | ||
| putStrLn $ colored yellow $ "The ack chan is not empty after 50ms, some acks may not make it to the server" |
Contributor
There was a problem hiding this comment.
Is this to stay in the test?
Comment on lines
+279
to
+282
| timeout 1_000_000 (wait wsThread) >>= \case | ||
| Nothing -> | ||
| putStrLn $ colored yellow $ "The websocket thread did not close after waiting for 1s" | ||
| Just () -> pure () |
Contributor
There was a problem hiding this comment.
Is this to stay in the test?
Comment on lines
+201
to
+211
| rsingleton0 :: forall x. x -> Range 0 1 [x] | ||
| rsingleton0 = rcast . rsingleton | ||
|
|
||
| rnil1 :: forall x. Range 0 1 [x] | ||
| rnil1 = rcast rnil | ||
|
|
||
| partitionToRange :: These a b -> (Range 0 1 [a], Range 0 1 [b]) | ||
| partitionToRange = \case | ||
| (This a) -> (rsingleton0 a, rnil1) | ||
| (That b) -> (rnil1, rsingleton0 b) | ||
| (These a b) -> (rsingleton0 a, rsingleton0 b) |
Contributor
There was a problem hiding this comment.
Do these belong in Data.Range?
mdimjasevic
changed the title
echoes-hq
bot
added
the
echoes: product-roadmap
…ti-mq-classic-queues-for-notifications
added 7 commits
October 23, 2024 09:33
…ti-mq-classic-queues-for-notifications
…ti-mq-classic-queues-for-notifications
…ti-mq-classic-queues-for-notifications
…ti-mq-classic-queues-for-notifications
…ti-mq-classic-queues-for-notifications
…ti-mq-classic-queues-for-notifications
mdimjasevic
changed the title
mdimjasevic
deleted the
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
branch
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is not to be merged before we make a release 5.6 per https://wearezeta.atlassian.net/browse/WPB-11239. The release happened on Oct 31, 2024: https://github.com/wireapp/wire-server/releases/tag/v2024-10-30
Checklist
changelog.derror,undefined,TODO, and fix thembrig currently loads a conn into Env and keeps it even after it dies. we should load an MVar into the env and replace broken chans in a top-level loop (like the old delete users queue in brig).(this point was based on a misunderstanding.)