Details

Reviewers

• karol
tomek
ashoat

Commits

rCOMM96e4dd08c898: [services] Tunnelbroker - Put and get messages from database in grpc handler

Summary

Message persisting logic in this diff:

On gRPC Send request we are storing a message to the dynamoDB database before the AMQP message is pushed to the RabbitMQ. In this case, we are making sure that the message is persisted and we don't rely on the RabbitMQ delivery only.
On gRPC Get request, we are retrieving the messages for the deviceID from the dynamoDB database first and delivering them to the device.
- After retrieving messages from the database, wait for the messages from the AMQP message broker (RabbitMQ);
- Follow-up messages are delivered as Get stream request messages;
- On message broker (RabbitMQ) message delivery, delete that message from the database by unique messageID.

Test Plan

Run yarn run-tunnelbroker-service and successfully built the service without any warnings and errors.
Message successfully delivered when clients are online:
- Client with deviceID#1 sends the message to deviceID#2 using the Send request;
- deviceID#2 is online and listening for the new messages using the Get stream;
- deviceID#2 instantly and successfully gets the message from deviceID#1 using the Get stream;
Message successfully delivered after client reconnects/offline:
- Client with deviceID#1 sends the message to deviceID#2 using the Send request;
- deviceID#2 is offline at that time;
- deviceID#2 connects to the tunnelbroker server and successfully gets the message from deviceID#1 using the Get stream;
Message successfully delivered when RabbitMQ server was restarted and the queue was cleared when the messages were sent, but the device was offline at that time and then reconnects after:
- Client with deviceID#1 sends the message to deviceID#2 using the Send request;
- deviceID#2 is offline at that time;
- RabbitMQ server goes down, the messages queue was cleared;
- RabbitMQ server goes up;
- deviceID#2 connects to the tunnelbroker server and successfully gets the message from deviceID#1 using the Get stream.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Apr 25 2022, 7:19 AM

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptApr 25 2022, 7:19 AM

• max added parent revisions: D3808: [services] Tunnelbroker - Message expiration fulfill moved to database manager's insert, D3830: [services] Tunnelbroker - Fix warnings during the AmqpManager build, D3239: [services] Tunnelbroker - DatabaseManager method to find messages by the receiver.Apr 25 2022, 7:20 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 25 2022, 7:20 AM

Harbormaster failed remote builds in B8507: Diff 11866!

Rebase.

Harbormaster completed remote builds in B8508: Diff 11867.Apr 25 2022, 8:11 AM

• max requested review of this revision.Apr 25 2022, 8:11 AM

Herald added a reviewer: • karol. · View Herald TranscriptApr 25 2022, 8:11 AM

• max planned changes to this revision.Apr 25 2022, 8:13 AM

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

• max edited the test plan for this revision. (Show Details)

• max added a reviewer: tomek.

I've tested it and it can be pushed to the review now as a part of the message persistence stack.

Fixing possible double delivery message bug, by clearing the delivery broker queue in case we have the messages in the database on the device new
connection.

Harbormaster completed remote builds in B8754: Diff 12197.May 4 2022, 6:21 AM

Rebase and merge on master due to the stack changes.

Harbormaster completed remote builds in B8775: Diff 12222.May 4 2022, 7:51 AM

• max added a child revision: D3914: [services] Tunnelbroker - Wrap gRPC Get message response into a function in Tunnelbroker.May 4 2022, 8:11 AM

ashoat requested changes to this revision.May 4 2022, 11:03 AM

ashoat added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
215 ↗	(On Diff #12222)	I'm confused, can you remind me why we `erase` here? If we want to clear the messages after they are sent, shouldn't we do that after we have a successful send?
225–226 ↗	(On Diff #12222)	How is this different from the `erase` above?
241–243 ↗	(On Diff #12222)	What is the point of erasing if something is empty?

This revision now requires changes to proceed.May 4 2022, 11:03 AM

I wanted to see the changes locally to perform what's in the test plan but I cannot pull this change.

arc patch D3833 results in errors. Can you please fix this?

Also, the test plan is a bit unclear to me. I don't get points 2 and 3. I was hoping that it becomes clear once I do it locally but that is impossible. Please, fix this revision.

• max planned changes to this revision.May 9 2022, 7:10 AM

In D3833#109876, @karol-bisztyga wrote:

I wanted to see the changes locally to perform what's in the test plan but I cannot pull this change.

arc patch D3833 results in errors. Can you please fix this?

I have created a new arc patch D3833 clear the build cache and the build is successful without errors. @karol-bisztyga can you please send an error that you have?

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
215 ↗	(On Diff #12222)	I'm confused, can you remind me why we `erase` here? If we want to clear the messages after they are sent, shouldn't we do that after we have a successful send? At this point, when a client connects and requests GET for the messages first we check if there are undelivered messages in the database. If so, we erase the messages to deliver from rabbitMQ which are handled by DeliveryBroker. We are doing so to get rid of possible double delivery because we can have messages in the database and same in the rabbitMQ queue (we have a low TTL in rabbitMQ messages, but the client can reconnect in a lower timeframe). Because all the undelivered messages exist in the database we can clear the rabbitMQ queue for the device.
225–226 ↗	(On Diff #12222)	How is this different from the `erase` above? Here we are erasing the message from the database. We are removing here from the database only after successfully delivering to the client, which in our case is successfully written to `writer`. That's why we can be sure that the message was delivered to the client before it was removed.
241–243 ↗	(On Diff #12222)	What is the point of erasing if something is empty? To handle multithreading we are storing messages in DeliveryBroker structure which is: `folly:ConcurrentHashMap[deviceID] -> folly::MPMCQueue` If deviceID messages queue is empty we don't need to store `folly::MPMCQueue` for it and can free memory to fix possible 'ghost' queues. That's why we are removing it by calling erase method which removes the `MPMCQueue` from `ConcurrentHashMap`.

ashoat added inline comments.May 12 2022, 12:47 PM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
215 ↗	(On Diff #12222)	Can you add a code comment explaining this? It seems non-obvious
225–226 ↗	(On Diff #12222)	We should wait until we can confirm delivery to the client (when the client responded with an updated checkpoint) before we delete from the database. I recognize that this is not the purpose of this diff... can you link a Linear task tracking the suggestion I made? Hopefully one already exists, but if not we should create one
241–243 ↗	(On Diff #12222)	Got it! Can you introduce a new method in `DeliveryBroker` called `deleteQueueIfEmpty` that runs these three lines? Then the code will be more self-documenting. It might still be good to add a code comment to `deleteQueueIfEmpty` to clarify that we're doing it to free memory

• max marked 5 inline comments as done.May 17 2022, 12:47 PM

• max added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
225–226 ↗	(On Diff #12222)	We should wait until we can confirm delivery to the client (when the client responded with an updated checkpoint) before we delete from the database. I recognize that this is not the purpose of this diff... can you link a Linear task tracking the suggestion I made? Hopefully one already exists, but if not we should create one Sure! I've created ENG-1158 task to follow on this comment.
241–243 ↗	(On Diff #12222)	Got it! Can you introduce a new method in `DeliveryBroker` called `deleteQueueIfEmpty` that runs these three lines? Then the code will be more self-documenting. It might still be good to add a code comment to `deleteQueueIfEmpty` to clarify that we're doing it to free memory Makes sense to me. I've created ENG-1146 task.

Accepting as my concerns have been addressed, but I think @karol-bisztyga's still need to be addressed too

In D3833#109876, @karol-bisztyga wrote:

I wanted to see the changes locally to perform what's in the test plan but I cannot pull this change.

arc patch D3833 results in errors. Can you please fix this?

Hey @karol-bisztyga, can you please be more precise about what kind of build error you have?
I have created a new arc patch D3833 clear the build cache and the build is successful without errors. Seems our CI built it without errors too.

In D3833#115283, @geekbrother wrote:

In D3833#109876, @karol-bisztyga wrote:

I wanted to see the changes locally to perform what's in the test plan but I cannot pull this change.

arc patch D3833 results in errors. Can you please fix this?

Hey @karol-bisztyga, can you please be more precise about what kind of build error you have?
I have created a new arc patch D3833 clear the build cache and the build is successful without errors. Seems our CI built it without errors too.

If it says parent commit could not be found, try running git pull --all --tags first

edit: looks like that should work

According to @geekbrother, @karol-bisztyga has been unresponsive over Comm. I'd like to unblock this diff.

I wanted to see the changes locally to perform what's in the test plan but I cannot pull this change.

arc patch D3833 results in errors. Can you please fix this?

Please see @atul's comment above. git pull --all --tags && arc patch D3833 works for me, @atul, and @geekbrother.

Also, the test plan is a bit unclear to me. I don't get points 2 and 3. I was hoping that it becomes clear once I do it locally but that is impossible. Please, fix this revision.

This comment from 2 weeks ago was never responded to. It should've been responded to 2 weeks ago.

• max mentioned this in D4100: [services] Tunnelbroker - Introducing `deleteQueueIfEmpty` method for DeliveryBroker.May 23 2022, 5:27 AM

• max removed parent revisions: D3239: [services] Tunnelbroker - DatabaseManager method to find messages by the receiver, D3830: [services] Tunnelbroker - Fix warnings during the AmqpManager build, D3808: [services] Tunnelbroker - Message expiration fulfill moved to database manager's insert.May 23 2022, 6:20 AM

• max added a parent revision: D4100: [services] Tunnelbroker - Introducing `deleteQueueIfEmpty` method for DeliveryBroker.

Rebase on master.
Update with using deleteQueueIfEmpty() DeliveryBroker method.
Code comments was added.

• max planned changes to this revision.May 23 2022, 6:24 AM

Harbormaster failed remote builds in B9387: Diff 13005!May 23 2022, 6:26 AM

• max edited the summary of this revision. (Show Details)May 25 2022, 6:21 AM

• max edited the test plan for this revision. (Show Details)

Rebase on master and tools namespace updates.

Harbormaster completed remote builds in B9466: Diff 13117.May 25 2022, 6:38 AM

In D3833#109876, @karol-bisztyga wrote:

I wanted to see the changes locally to perform what's in the test plan but I cannot pull this change.

arc patch D3833 results in errors. Can you please fix this?

Sorry about this, fixed!

Also, the test plan is a bit unclear to me. I don't get points 2 and 3. I was hoping that it becomes clear once I do it locally but that is impossible. Please, fix this revision.

I've updated the description and test plan, hope now it's more clear.

According to @geekbrother, @karol-bisztyga has been unresponsive over Comm. I'd like to unblock this diff.

Not sure what you're talking about. I got pinged just today about this on the comm app. I'm not recalling any messages from the past though. I try to respond to all messages, @geekbrother what's this about?

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
220 ↗	(On Diff #13117)	Nit: we could define this outside of the loop.

Add comment for the deleteQueueIfEmpty().
tunnelbroker::GetResponse in loop usage optimization.

• max marked an inline comment as done.May 25 2022, 8:00 AM

• max added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
220 ↗	(On Diff #13117)	Nit: we could define this outside of the loop. Nice optimization catch! We can use a single instance and use grpc generated `Clear()` method on it.

Harbormaster completed remote builds in B9470: Diff 13121.May 25 2022, 8:04 AM

tomek accepted this revision.May 27 2022, 2:19 AM

tomek added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
165 ↗	(On Diff #13121)	Maybe we should consider changing this method's argument to be `MessageItem`? It might require moving `MessageItem` from `database` namespace.

Pinging @ashoat for a re-review of this diff.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
165 ↗	(On Diff #13121)	Maybe we should consider changing this method's argument to be `MessageItem`? It might require moving `MessageItem` from `database` namespace. I was thinking about this too. I'll create a task for this in the backlog.

ashoat accepted this revision.May 30 2022, 12:14 PM

This revision is now accepted and ready to land.May 30 2022, 12:14 PM

Closed by commit rCOMM96e4dd08c898: [services] Tunnelbroker - Put and get messages from database in grpc handler (authored by • max). · Explain WhyMay 31 2022, 10:08 AM

This revision was automatically updated to reflect the committed changes.

• max marked an inline comment as done.

• max added a commit: rCOMM96e4dd08c898: [services] Tunnelbroker - Put and get messages from database in grpc handler.

[services] Tunnelbroker - Put and get messages from database in grpc handler
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 13254

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

[services] Tunnelbroker - Put and get messages from database in grpc handlerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 13254

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

[services] Tunnelbroker - Put and get messages from database in grpc handler
ClosedPublic
Actions

Revision Contents
Changeset List