[services] Tunnelbroker - Fix table names to use config-only instead of constant
ClosedPublic
Actions

Authored by • max on Aug 1 2022, 7:15 AM.

Details

Reviewers

• karol
tomek

Commits

rCOMMe15250da890e: [services] Tunnelbroker - Fix table names to use config-only instead of constant

Summary

This diff is a fix.
Fix to use table names from config-only everywhere in the codebase instead of constant with the default table name. This will fix the bug when the default table name from constants is used instead of the table name for the test because in the sandbox mode Tunnelbroker reads the corresponding config file from services/tunnelbroker/tunnelbroker-dev.ini.

Linear task: ENG-1494

Test Plan

Run tests in a sandbox using the yarn run-unit-tests and use the table names from the services/tunnelbroker/tunnelbroker-dev.ini config file.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Aug 1 2022, 7:15 AM

• max held this revision as a draft.

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

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

• max added parent revisions: D4693: [services] Tunnelbroker - Fix `IDs` in Terraform dynamodb table attributes, D4694: [services] Tunnelbroker - Change `findMessageItem`, `removeMessageItem` to use composite key, D4695: [services] Tunnelbroker - Remove `ToDeviceID` index from messages table.

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

• max added reviewers: • karol, tomek.

Harbormaster completed remote builds in B11003: Diff 15137.Aug 1 2022, 7:27 AM

tomek requested changes to this revision.Aug 2 2022, 9:14 AM

tomek added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
187 ↗	(On Diff #15137)	This sounds risky, as we don't verify that all the messages have the same table name. Overall, it seems like tableName should be a static property and not something associated with message instances. So some questions: Why do we need a message item to have a table name? Can two messages have different table names? Where is this name taken from? If we say in this diff that we take this from config, why do we take this from a message?

This revision now requires changes to proceed.Aug 2 2022, 9:14 AM

• max planned changes to this revision.Aug 2 2022, 12:06 PM

• max requested review of this revision.Aug 3 2022, 6:29 AM

• max added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
187 ↗	(On Diff #15137)	This sounds risky, as we don't verify that all the messages have the same table name. Actually, it doesn't, because the table name for every instance is the same and comes from config, and it can't be changed from the entity side. It can be different only in a different app running instances, because of config file loads on a start only. Overall, it seems like tableName should be a static property and not something associated with message instances. Agree with you, but this practice is a follow-up from blob and backup services, where the entity has a method to get the table name. So I just followed it. So some questions: Why do we need a message item to have a table name? This is a follow-up practice from the blob and backup, we use get table name method in many internal functions. Can two messages have different table names? No, because it comes from the configManager and it's a same for every message item. Where is this name taken from? It comes from the configManager and config file respectively. But there are a different config files in a normal and sandbox modes. If we say in this diff that we take this from config, why do we take this from a message? It's a good idea to use a single point of truth here and not copy paste config's `getParamater()` from getTableName to here. In case something will change in a future we will need to update only one `getTableName()` and not every config `getParameter()` for a table name.

I still think that getTableName should be a static method instead of the instance one. Accepting, because this diff only shows the issue and not introduces it.

This revision is now accepted and ready to land.Aug 3 2022, 9:30 AM

In D4696#136055, @tomek wrote:

I still think that getTableName should be a static method instead of the instance one. Accepting, because this diff only shows the issue and not introduces it.

We should change this in all services. I've created ENG-1542 as a follow-up on this catch,

Merge on master changes.

This revision was landed with ongoing or failed builds.Aug 3 2022, 9:50 AM

Closed by commit rCOMMe15250da890e: [services] Tunnelbroker - Fix table names to use config-only instead of constant (authored by • max). · Explain Why

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMMe15250da890e: [services] Tunnelbroker - Fix table names to use config-only instead of constant.

Harbormaster completed remote builds in B11093: Diff 15280.Aug 3 2022, 9:56 AM