Paths

Table of Contentst

[services] Tunnelbroker - Database function that removes messages by a batch using messageIDs and receiver DeviceID.
ClosedPublic
Actions

Authored by • max on Jul 7 2022, 7:43 AM.

Details

Reviewers

• karol
tomek

Commits

rCOMM5bd8a63d6277: [services] Tunnelbroker - Database function that removes messages by a batch…

Summary

We need effectively remove messages in a batch from the DynamoDB database using a composite key (messageID + toDeviceID). For this task, we can use the inner BatchWriteItem function from ENG-1302 and delete items by batches instead of 1 by 1.

Linear task: ENG-1361

Test Plan

Run test introduced in D4478

Diff Detail

Repository

rCOMM Comm

Branch

delete-messages-by-composite-key

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Jul 7 2022, 7:43 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJul 7 2022, 7:43 AM

• max edited the summary of this revision. (Show Details)Jul 7 2022, 7:45 AM

• max added parent revisions: D4475: [services] Tunnelbroker - Switching to use composite key in a messageItem entity, D4473: [services] Tunnelbroker - Switching to use composite key in a messages table in Terraform, D4372: [services] Tunnelbroker - Add DYNAMODB_MAX_BATCH_ITEMS to Constants, D4436: [services] Tunnelbroker - Add exponential backoff related constants, D4373: [services] Inner function to write to DynamoDB by batch.

• max mentioned this in D4478: [services] Tunnelbroker - Add test for `removeMessageItemsByIDsForDeviceID` database function.Jul 7 2022, 8:32 AM

• max edited the test plan for this revision. (Show Details)Jul 7 2022, 8:32 AM

• max published this revision for review.Jul 7 2022, 8:37 AM

• max added a child revision: D4478: [services] Tunnelbroker - Add test for `removeMessageItemsByIDsForDeviceID` database function.

• max added reviewers: • karol, tomek.

• max mentioned this in D4220: [services] Tunnelbroker - Add remove MessageItems older than checkpoint database method.Jul 7 2022, 8:45 AM

• max mentioned this in D4221: [services] Tunnelbroker - Add test for the RemoveMessageItemsOlderThenCheckpoint database method.

Harbormaster failed remote builds in B10375: Diff 14283!Jul 7 2022, 8:47 AM

tomek accepted this revision.Jul 8 2022, 2:32 AM

tomek added 1 blocking reviewer(s): • karol.

tomek added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
224–226 ↗	(On Diff #14283)	Why do you prepend the name with `cur`?

• max planned changes to this revision.Jul 11 2022, 2:03 AM

Rebased on parents changes. cur* variable were changed to current*.

services/tunnelbroker/src/Database/DatabaseManager.cpp
224–226 ↗	(On Diff #14283)	Why do you prepend the name with `cur`? It's shorthand for 'current'. I've changed it to `currentWriteRequest` instead.

Harbormaster completed remote builds in B10442: Diff 14386.Jul 11 2022, 2:49 AM

Please fix the typo in this diff's title:

[services] Tunnelbroker - Database function that removes messages in a batch using the composite key and batchWrite

services/tunnelbroker/src/Database/DatabaseManager.cpp
216–227	Maybe we could create `deleteRequest` before the `for` loop? And inside of that loop, we could just change the message id. That would result in a lot fewer allocations, right? Curious for your perspective

This revision now requires changes to proceed.Jul 11 2022, 3:34 AM

• max marked an inline comment as done.Jul 12 2022, 3:02 PM

• max added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
216–227	Maybe we could create `deleteRequest` before the `for` loop? And inside of that loop, we could just change the message id. That would result in a lot fewer allocations, right? Curious for your perspective There is a reason why I've put it inside the loop and we re-create it every loop: If we have the `Aws::DynamoDB::Model::DeleteRequest deleteRequest` outside of the loop and then use `deleteRequest.AddKey(...)` inside the loop we will always delete only the first key-value because `AddKey(...)` uses emplace to populate an inner map. In this case, the only first kay-value pair will be inserted because `emplace` will ignore the next key inserts (because the key already exists in the map from the previous loop). We can possibly use it outside of the loop if we have the ability to clear the inner map, but SDK doesn't have a such method. In another way, we can use `SetKey(...)` instead of `AddKey(...)`, but this changes don't provide any improvement in this case.

In D4476#128010, @karol-bisztyga wrote:
Please fix the typo in this diff's title:
[services] Tunnelbroker - Database function that removes messages in a batch using the composite key and batchWrite

Thanks for catching it!

• karol accepted this revision.Jul 19 2022, 2:19 AM

This revision is now accepted and ready to land.Jul 19 2022, 2:19 AM

Rebase on master.

This revision was landed with ongoing or failed builds.Jul 25 2022, 6:45 AM

Closed by commit rCOMM5bd8a63d6277: [services] Tunnelbroker - Database function that removes messages by a batch… (authored by • max). · Explain Why

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM5bd8a63d6277: [services] Tunnelbroker - Database function that removes messages by a batch….

Harbormaster completed remote builds in B10795: Diff 14843.Jul 25 2022, 6:49 AM

• max mentioned this in rCOMMeec927757d3d: [services] Tunnelbroker - Add test for `removeMessageItemsByIDsForDeviceID`….Aug 1 2022, 12:13 PM