Paths

Table of Contentst

[services] Tunnelbroker - DatabaseManager method to find messages by the receiver
ClosedPublic
Actions

Authored by • max on Feb 16 2022, 4:53 PM.

Details

Reviewers

tomek
• karol

Commits

rCOMMd4d31dcce367: [services] Tunnelbroker - DatabaseManager method to find messages by the…

Summary

We need to fetch undelivered messages from the database by the receiver which is toDeviceID field in our case.
findMessageItemsByReceiever method was implemented for this purpose.

Related linear task: ENG-751

Architecture review for the tunnelbroker.

Test Plan

Unit tests are on D3781

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Feb 16 2022, 4:53 PM

Herald added subscribers: • karol, atul, • adrian and 2 others. · View Herald TranscriptFeb 16 2022, 4:53 PM

• max edited the summary of this revision. (Show Details)Feb 16 2022, 4:57 PM

• max added reviewers: tomek, • karol.

Harbormaster completed remote builds in B6906: Diff 9758.Feb 16 2022, 4:58 PM

• max requested review of this revision.Feb 16 2022, 4:58 PM

Hey @geekbrother is there anything like a DB structure schema described somewhere for the tunnelbroker? I bet you understand it, but it would be cool to have it in one place for other people. You can look at D2950 something like this would be great... It would be really helpful to review diffs like this and in general to understand how the service works. For now, if I want to figure this out, I have to:

go through the dynamo DB console on AWS - to see what's the primary keys(you didn't add my fix for primary keys as it is blocked by earlier diffs from the stack so the primary keys cannot be figured out from the code)
go through the DB entities file - to figure out what are all fields in every one of them
considering how many tables you created, I'd have to do such a description myself to understand this(which is BTW an anti-pattern to create multiple tables for one service in dynamo DB without a really strong reason. It would be good to have a full DB description to rediscuss this).

Please, add a DB structure somewhere(or point it out if I'm not seeing it but it's somewhere there) so I can review this diff. Without it, I cannot see if the index you created is correct.

This revision now requires changes to proceed.Feb 17 2022, 1:24 AM

• max planned changes to this revision.Feb 17 2022, 7:39 AM

In D3239#86334, @karol-bisztyga wrote:

Hey @geekbrother is there anything like a DB structure schema described somewhere for the tunnelbroker? I bet you understand it, but it would be cool to have it in one place for other people. You can look at D2950 something like this would be great... It would be really helpful to review diffs like this and in general to understand how the service works. For now, if I want to figure this out, I have to:

go through the dynamo DB console on AWS - to see what's the primary keys(you didn't add my fix for primary keys as it is blocked by earlier diffs from the stack so the primary keys cannot be figured out from the code)

go through the DB entities file - to figure out what are all fields in every one of them

considering how many tables you created, I'd have to do such a description myself to understand this(which is BTW an anti-pattern to create multiple tables for one service in dynamo DB without a really strong reason. It would be good to have a full DB description to rediscuss this).

Please, add a DB structure somewhere(or point it out if I'm not seeing it but it's somewhere there) so I can review this diff. Without it, I cannot see if the index you created is correct.

That makes sense to me.
I've updated architecture review for the tunnelbroker. Please take a look at DynamoDB database model section.

Herald added a subscriber: • benschac. · View Herald TranscriptMar 24 2022, 7:20 AM

• max requested review of this revision.Mar 24 2022, 7:21 AM

• max edited the summary of this revision. (Show Details)

@karol-bisztyga it looks like this diff is waiting for your review

Accepting but I see 2 problems here:

I cannot see the index on the aws, is it there?
Is this index documented somewhere? From here we do not know what it projects, etc.

Maybe I missed something but would be cool to get responses to these questions before landing.

This revision is now accepted and ready to land.Mar 30 2022, 6:50 AM

• max planned changes to this revision.Apr 7 2022, 1:50 AM

In D3239#97178, @karol-bisztyga wrote:

Accepting but I see 2 problems here:

I cannot see the index on the aws, is it there?

Fixed, thanks!

Is this index documented somewhere? From here we do not know what it projects, etc.

I've updated the Tunnelbroker architecture review with the database model.

This revision is now accepted and ready to land.Apr 18 2022, 7:43 AM

Eventually the database model should be checked in and live in code (Terraform)

Fixed nit in a function name. Index name moved to the item along with the field names.

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

This revision now requires review to proceed.Apr 19 2022, 6:49 AM

Re-requesting review here because of some minor but changes:

Fixed a nit in a function name: Receiever -> Receiver.
Moved index name to the MessageItem along with the field constants.

Harbormaster failed remote builds in B8308: Diff 11604!Apr 19 2022, 6:56 AM

• max retitled this revision from [services][tunnelbroker] DatabaseManager method to find messages by the receiver. to [services][tunnelbroker] DatabaseManager method to find messages by the receiver.Apr 19 2022, 6:57 AM

• max added a child revision: D3781: [services] Tunnelbroker - Database findMessageItemsByReceiver tests.Apr 19 2022, 7:03 AM

• max mentioned this in D3781: [services] Tunnelbroker - Database findMessageItemsByReceiver tests.

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

• max retitled this revision from [services][tunnelbroker] DatabaseManager method to find messages by the receiver to [services] Tunnelbroker - DatabaseManager method to find messages by the receiver.

Looks ok. Added @karol-bisztyga as a blocking reviewer because he was reviewing the first revision.

How is ENG-751 related to this?

Is this index documented somewhere? From here we do not know what it projects, etc.

I've updated the Tunnelbroker architecture review with the database model.

Sorry, but I cannot see anything about this index in your arch review you posted :( Where is it? There's a DB model, but nothing about indexes is there.

I see however that you created this index on the AWS. So, based on what's there, I'm going to answer my question for you: the index projects all fields.

services/tunnelbroker/src/Database/MessageItem.h
28 ↗	(On Diff #11604)	Good idea to store indexes here

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

• max added a child revision: D3833: [services] Tunnelbroker - Put and get messages from database in grpc handler.Apr 25 2022, 7:20 AM

Rebase on master to resolve merge conflicts.

Harbormaster completed remote builds in B8745: Diff 12185.May 4 2022, 12:02 AM

In D3239#106211, @karol-bisztyga wrote:

How is ENG-751 related to this?

Is this index documented somewhere? From here we do not know what it projects, etc.

I've updated the Tunnelbroker architecture review with the database model.

Sorry, but I cannot see anything about this index in your arch review you posted :( Where is it? There's a DB model, but nothing about indexes is there.

I see however that you created this index on the AWS. So, based on what's there, I'm going to answer my question for you: the index projects all fields.