[services] Tunnelbroker - Change `findMessageItem`, `removeMessageItem` to use composite key
ClosedPublic
Actions

Authored by • max on Aug 1 2022, 6:58 AM.

Details

Reviewers

• karol
tomek

Commits

rCOMM77ae0b9477b8: [services] Tunnelbroker - Change `findMessageItem`, `removeMessageItem` to use…

Summary

As we changed the messages table model to use a composite key in ENG-1362 we can use a composite key instead of an index in findMessageItem, removeMessageItem methods to improve database lookup performance.

Linear task: ENG-1493

Test Plan

Run database-related unit tests using yarn run-unit-tests

Diff Detail

Repository

rCOMM Comm

Branch

change-findMessageItem-removeMessageItem-to-composite-key

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Aug 1 2022, 6:58 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptAug 1 2022, 6:58 AM

• max added a parent revision: D4693: [services] Tunnelbroker - Fix `IDs` in Terraform dynamodb table attributes.Aug 1 2022, 6:59 AM

• max added reviewers: • karol, tomek.

• max published this revision for review.Aug 1 2022, 7:02 AM

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

• max added a child revision: D4695: [services] Tunnelbroker - Remove `ToDeviceID` index from messages table.Aug 1 2022, 7:07 AM

Harbormaster completed remote builds in B11001: Diff 15135.Aug 1 2022, 7:11 AM

• max added a child revision: D4696: [services] Tunnelbroker - Fix table names to use config-only instead of constant.Aug 1 2022, 7:17 AM

Run database-related unit tests using yarn run-unit-tests

If we're relying on the tests, we need to make sure that they are really helpful. If the tests were passing before this change and after it, are we really sure that our change changes anything? The solution to that issue is to introduce a test which would be failing with the older code but is now passing - do you think it is possible to have such a test?

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

In D4694#135372, @tomek wrote:

Run database-related unit tests using yarn run-unit-tests

If we're relying on the tests, we need to make sure that they are really helpful. If the tests were passing before this change and after it, are we really sure that our change changes anything? The solution to that issue is to introduce a test which would be failing with the older code but is now passing - do you think it is possible to have such a test?

Thanks for a comment @tomek ! The test is passing because the test file is changed too due to the function changes. I don't think we need another test that fails if using old methods instead because the updated tests will fail.

Closed by commit rCOMM77ae0b9477b8: [services] Tunnelbroker - Change `findMessageItem`, `removeMessageItem` to use… (authored by • max). · Explain WhyAug 2 2022, 9:30 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM77ae0b9477b8: [services] Tunnelbroker - Change `findMessageItem`, `removeMessageItem` to use….

In D4694#135499, @max wrote:

In D4694#135372, @tomek wrote:

Run database-related unit tests using yarn run-unit-tests

If we're relying on the tests, we need to make sure that they are really helpful. If the tests were passing before this change and after it, are we really sure that our change changes anything? The solution to that issue is to introduce a test which would be failing with the older code but is now passing - do you think it is possible to have such a test?

Thanks for a comment @tomek ! The test is passing because the test file is changed too due to the function changes. I don't think we need another test that fails if using old methods instead because the updated tests will fail.