Details

Reviewers

tomek
• karol

Commits

rCOMM0794baa35893: [services] Tunnelbroker - Fix delivering messages one by one and remove mutex.

Summary

Using messageID to remove a certain message from the delivery queue, instead of using a receiver deviceID to remove all messages for the device.

1. Message structure changed

To achieve this the DeliveryBroker internal message structure needs to be changed from the current:

map[reciever deviceID] → vector<Message information>

to:

map[receiver deviceID] → Queue → Message information.

2. Conditional variables+mutexes became folly:: UnboundedQueue.

Conditional variables and mutex were removed in favor of folly:: UnboundedQueue

3. Remove is became pop as we are using a queue instead of a map.

The remove function changed to pop to remove the first message from the queue.

4. Function names usage refactoring due to the above changes.

Usage of the functions is also refactored in AMQP and gRPC handlers.

Review notes:

These diffs changes are closely interconnected with each other and it's effective for review to use one diff instead of splitting it. This is a smaller chunk of changes that can be buildable. Removing mutexes is closely interconnected with the structural changes that's why it is related to two tasks simultaneously.

Related linear tasks:

Test Plan

Run yarn run-tunnelbroker-service and successfully built the service.
Messages are delivered one by one and removed from the queue.
Passed unit tests on D3645.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

It sounds like a good idea, but there's one important issue. By using unordered_map we're losing an information about ordering of the messages. I assume, that the delivery broker should work like FIFO, but after this change the ordering becomes random. It might be also possible that we don't need to keep the ordering... but that sounds unlikely.

This revision now requires changes to proceed.Feb 25 2022, 4:14 AM

• max planned changes to this revision.Mar 2 2022, 1:10 PM

In D3279#88399, @palys-swm wrote:

It sounds like a good idea, but there's one important issue. By using unordered_map we're losing an information about ordering of the messages. I assume, that the delivery broker should work like FIFO, but after this change the ordering becomes random. It might be also possible that we don't need to keep the ordering... but that sounds unlikely.

Good catch! I've used unorder_map for the performance reason, but it looks like a bigger issue.

It might be also possible that we don't need to keep the ordering... but that sounds unlikely.

Yes, we need to keep the order of messages. Will fix it.

Herald added a subscriber: • benschac. · View Herald TranscriptMar 31 2022, 7:57 AM

Switching to MPMCQueue and remove mutex and conditional variables.

• max retitled this revision from [services] Tunnelbroker - Fix delivering messages one by one to [services] Tunnelbroker - Fix delivering messages one by one and remove mutex..Apr 4 2022, 7:13 AM

Harbormaster failed remote builds in B7830: Diff 10998!Apr 4 2022, 7:18 AM

In D3279#88399, @palys-swm wrote:

It sounds like a good idea, but there's one important issue. By using unordered_map we're losing an information about ordering of the messages. I assume, that the delivery broker should work like FIFO, but after this change the ordering becomes random. It might be also possible that we don't need to keep the ordering... but that sounds unlikely.

Switching to ConcurrentHashmap<MPMCQueue> in this case we are preserving the order of the messages and can add and pop messages by the pointer.

• max planned changes to this revision.Apr 4 2022, 10:02 AM

Fixing build error due to pointers.

Harbormaster completed remote builds in B7843: Diff 11011.Apr 4 2022, 12:36 PM

• max planned changes to this revision.Apr 5 2022, 1:04 AM

Switch to use UnboundedQueue instead of MPMCQueue.

Harbormaster completed remote builds in B7910: Diff 11089.Apr 6 2022, 1:25 AM

• max edited the summary of this revision. (Show Details)Apr 6 2022, 1:32 AM

Overall, it should be a stack instead of a diff. We could e.g. start with storing message id, then introduce the queue and then remove synchronization. But for now, let's keep it.

services/tunnelbroker/docker-server/contents/server/src/DeliveryBroker/DeliveryBroker.cpp
18–27 ↗	(On Diff #11089)	Is it possible to assign `this->messagesMap.find(toDeviceID)` to a variable so that we don't need to call `find` twice?
44–47 ↗	(On Diff #11089)	It would be a good idea to explain why do we insert a new queue when we can't find one (it looks like we do that so that we can call blocking `pop` from `TunnelbrokerServiceImpl` and why we're not removing the queue when it becomes empty (we're calling `erase` from `TunnelbrokerServiceImpl`)
services/tunnelbroker/docker-server/contents/server/src/Service/TunnelbrokerServiceImpl.cpp
212–214 ↗	(On Diff #11089)	Are you sure this is correct? Do we want to `erase` only when the queue is not empty? Shouldn't we inverse this condition?

This revision now requires changes to proceed.Apr 6 2022, 6:34 AM

• max planned changes to this revision.Apr 6 2022, 11:04 PM

Fix erase condition. Fix tools new namespace error.

• max planned changes to this revision.Apr 7 2022, 5:27 AM

• max added a child revision: D3645: [services] Tunnelbroker - DeliveryBroker tests.Apr 7 2022, 6:02 AM

Harbormaster completed remote builds in B7963: Diff 11167.Apr 7 2022, 8:14 AM

• max added a child revision: D3653: [services] Tunnelbroker - AMQP tests.Apr 7 2022, 9:06 AM

• max mentioned this in D3645: [services] Tunnelbroker - DeliveryBroker tests.Apr 18 2022, 5:20 AM

Following the discussion at ENG-609, a max size check for the delivery broker queue was added.

Harbormaster failed remote builds in B8259: Diff 11550!Apr 18 2022, 7:39 AM

• max planned changes to this revision.Apr 18 2022, 8:06 AM

Fixed build failure nit.

• max planned changes to this revision.Apr 18 2022, 12:34 PM

Harbormaster failed remote builds in B8271: Diff 11566!Apr 18 2022, 12:39 PM

Harbormaster completed remote builds in B8271: Diff 11566.Apr 18 2022, 12:47 PM

• max requested review of this revision.Apr 19 2022, 1:56 AM

• max marked 3 inline comments as done.

• max added inline comments.

services/tunnelbroker/docker-server/contents/server/src/DeliveryBroker/DeliveryBroker.cpp
18–27 ↗	(On Diff #11089)	Is it possible to assign `this->messagesMap.find(toDeviceID)` to a variable so that we don't need to call `find` twice? Thanks for the catch here. I've tried this in the playground, but it seems that holding the iterator here from `this->messagesMap.find(toDeviceID)` or holding a pointer to the unique pointer to the queue from `this->messagesMap.find(toDeviceID)->second` is like overengineering here for the second call of the find and seems no any performance benefits from it. Btw in this case it's thread-safe to hold an iterator. So, I'm not sure it is worth it.
44–47 ↗	(On Diff #11089)	It would be a good idea to explain why do we insert a new queue when we can't find one (it looks like we do that so that we can call blocking `pop` from `TunnelbrokerServiceImpl` You are right here. We are inserting the queue for the blocking wait in case we listen first before the insert happens. That's why we need the queue to exist and insert it. why we're not removing the queue when it becomes empty (we're calling `erase` from `TunnelbrokerServiceImpl`) The use case for the `erase` is when we need to remove the whole queue for the certain deviceID without popping each message from the upper layer (grpc handler, database handler). why we're not removing the queue when it becomes empty I think the control of erasing here must be at the upper level, which gives us the flexibility in this case for the optimization and not "insert-remove" the queue every time when the queue is empty for a certain device. Like, remove it when the device logs off.
services/tunnelbroker/docker-server/contents/server/src/Service/TunnelbrokerServiceImpl.cpp
212–214 ↗	(On Diff #11089)	Are you sure this is correct? Do we want to `erase` only when the queue is not empty? Shouldn't we inverse this condition? Fixed, great thanks for this catch!

• max added 1 blocking reviewer(s): tomek.Apr 19 2022, 2:09 AM

tomek requested changes to this revision.Apr 19 2022, 2:13 AM

tomek added inline comments.

services/tunnelbroker/docker-server/contents/server/src/DeliveryBroker/DeliveryBroker.cpp
44–47 ↗	(On Diff #11089)	It would be a good idea to explain why do we insert a new queue when we can't find one (it looks like we do that so that we can call blocking `pop` from `TunnelbrokerServiceImpl` You are right here. We are inserting the queue for the blocking wait in case we listen first before the insert happens. That's why we need the queue to exist and insert it. why we're not removing the queue when it becomes empty (we're calling `erase` from `TunnelbrokerServiceImpl`) The use case for the `erase` is when we need to remove the whole queue for the certain deviceID without popping each message from the upper layer (grpc handler, database handler). why we're not removing the queue when it becomes empty I think the control of erasing here must be at the upper level, which gives us the flexibility in this case for the optimization and not "insert-remove" the queue every time when the queue is empty for a certain device. Like, remove it when the device logs off. Please explain that in the code comments - this isn't too obvious what happens here and we should save the maintainers time figuring out what's going on.

This revision now requires changes to proceed.Apr 19 2022, 2:13 AM

A comment about pop implementation was added.

• max added 1 blocking reviewer(s): tomek.Apr 19 2022, 3:23 AM

• max marked an inline comment as done.Apr 19 2022, 3:26 AM

• max added inline comments.

services/tunnelbroker/docker-server/contents/server/src/DeliveryBroker/DeliveryBroker.cpp
44–47 ↗	(On Diff #11089)	Please explain that in the code comments - this isn't too obvious what happens here and we should save the maintainers time figuring out what's going on. The comment about the `pop` implementation and adding an empty queue makes sense to me. I've added it. Not sure we need comments about `erase` because it's a one-line function with the usage implementation in the code.

Harbormaster completed remote builds in B8303: Diff 11599.Apr 19 2022, 3:32 AM

• max planned changes to this revision.Apr 20 2022, 6:36 AM

• max marked an inline comment as done.

Droping back to MPMCQueue due to the ENG-609 discussion.

• max edited the test plan for this revision. (Show Details)Apr 20 2022, 7:22 AM

Harbormaster completed remote builds in B8353: Diff 11663.Apr 20 2022, 7:26 AM

Looks ok! It might be a good idea for @karol-bisztyga to also take a look

I'm confused by the description here.

Using messageID to remove a certain message from the delivery queue, instead of using a receiver deviceID to remove all messages for the device.

We don't use message id for removing (or did I miss something?): erase uses deviceID.

It's good that you got rid of cond vars etc but can't we delete messageID? Why exactly do we need it?

If we really need it, I believe we can move the code that generates it to the inner scope as I suggested in the inline comment.

With all that said, I may be missing a bigger picture here so feel free to explain what exactly is going on. Thanks!

Back to your queue.

services/tunnelbroker/src/Amqp/AmqpManager.h
26–27 ↗	(On Diff #11663)	To be honest, it would be more logical
services/tunnelbroker/src/DeliveryBroker/DeliveryBroker.cpp
45 ↗	(On Diff #11663)
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
157 ↗	(On Diff #11663)	Do we need this here? Cannot we generate this id in the `send`'s scope?

This revision now requires changes to proceed.Apr 25 2022, 2:28 AM

Fix nit in the comment, send() parameters order was changed.

Harbormaster completed remote builds in B8494: Diff 11840.Apr 25 2022, 4:04 AM

In D3279#106203, @karol-bisztyga wrote:

I'm confused by the description here.

Using messageID to remove a certain message from the delivery queue, instead of using a receiver deviceID to remove all messages for the device.

We don't use message id for removing (or did I miss something?): erase uses deviceID.

Yes, we don't. MessageID is inside the queue structure and we are popping the message from it without knowing the messageID. We need this messageID later (after it was popped) for removing the certainly delivered message from the database and using it as a unique message identifier.

It's good that you got rid of cond vars etc but can't we delete messageID? Why exactly do we need it?

If we really need it, I believe we can move the code that generates it to the inner scope as I suggested in the inline comment.

Reply to the comment about the scope.
We removed the whole queue by the erase method to clean the queue for the certain deviceID without popping the message on-by-one from the queue and free the memory.

Hope I've added additional context here...

services/tunnelbroker/src/Amqp/AmqpManager.h
26–27 ↗	(On Diff #11663)	To be honest, it would be more logical Let's go with this parameters order. Changed it.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
157 ↗	(On Diff #11663)	Do we need this here? Cannot we generate this id in the `send`'s scope? We need this messageID generated at this scope for future message storing in the database. I think moving it inside and then using some getter is over-engineering here because we need this ID for the database storing item and they must be the same.

Thanks for clarifying, please answer that last question so I can fully understand.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
157 ↗	(On Diff #11663)	for future message storing in the database So this "message storing" is going to be implemented in this scope here, right?

This revision is now accepted and ready to land.Apr 25 2022, 4:36 AM

In D3279#106247, @karol-bisztyga wrote:

Thanks for clarifying, please answer that last question so I can fully understand.

Thanks for the fast reply ;)

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
157 ↗	(On Diff #11663)	for future message storing in the database So this "message storing" is going to be implemented in this scope here, right? Yes, right. That's why we need this generated messageID at this scope.

Rebase on master.

Harbormaster completed remote builds in B8499: Diff 11848.Apr 25 2022, 5:15 AM

Closed by commit rCOMM0794baa35893: [services] Tunnelbroker - Fix delivering messages one by one and remove mutex. (authored by • max). · Explain WhyApr 25 2022, 5:16 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM0794baa35893: [services] Tunnelbroker - Fix delivering messages one by one and remove mutex..

[services] Tunnelbroker - Fix delivering messages one by one and remove mutex.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 11849

services/tunnelbroker/src/Amqp/AmqpManager.h

services/tunnelbroker/src/Amqp/AmqpManager.cpp

services/tunnelbroker/src/Constants.h

services/tunnelbroker/src/DeliveryBroker/DeliveryBroker.h

services/tunnelbroker/src/DeliveryBroker/DeliveryBroker.cpp

services/tunnelbroker/src/DeliveryBroker/DeliveryBrokerEntites.h

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

[services] Tunnelbroker - Fix delivering messages one by one and remove mutex.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 11849

services/tunnelbroker/src/Amqp/AmqpManager.h

services/tunnelbroker/src/Amqp/AmqpManager.cpp

services/tunnelbroker/src/Constants.h

services/tunnelbroker/src/DeliveryBroker/DeliveryBroker.h

services/tunnelbroker/src/DeliveryBroker/DeliveryBroker.cpp

services/tunnelbroker/src/DeliveryBroker/DeliveryBrokerEntites.h

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

[services] Tunnelbroker - Fix delivering messages one by one and remove mutex.
ClosedPublic
Actions

Revision Contents
Changeset List