Page MenuHomePhabricator

[tunnelbroker] Fix `MESSAGES_TABLE_NAME` constant
ClosedPublic

Authored by max on Feb 15 2022, 8:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 29, 10:20 AM
Unknown Object (File)
Wed, Jun 26, 3:58 PM
Unknown Object (File)
Mon, Jun 24, 9:58 AM
Unknown Object (File)
Mon, Jun 24, 9:01 AM
Unknown Object (File)
Mon, Jun 24, 5:29 AM
Unknown Object (File)
Fri, Jun 21, 9:22 PM
Unknown Object (File)
Sun, Jun 16, 2:03 PM
Unknown Object (File)
Sun, Jun 16, 2:03 PM

Details

Summary

Was getting the following:

tunnelbroker-server    | terminate called after throwing an instance of 'std::runtime_error'
tunnelbroker-server    |   what():  Error: AWS DynamoDB table 'tunnelbroker-message' is not available

Checked the AWS console and it looks like the messages table name is tunnelbroker-messages instead of tunnelbroker-message?

Update:

Following our code convention about database names, fix dynamoDB table names to plural.

Linear task: ENG-1091

Landing procedure:

After landing, the running service in the AWS cloud needs to be restarted/updated using the newly built docker image.
The old tables must be removed from the AWS DynamoDB before the new container will be restarted.

Test Plan

No longer getting the above error.
Run yarn run-tunnelbroker-service and use new table names.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Feb 15 2022, 9:22 PM

We have an agreement not to use plural names in table names, that's why in our codebase we are using not plural names.
There is no data stored in the table yet and the table can be just renamed. I've created 'tunnelbroker-message' table name in DynamoDB and there are no errors. I think this diff needs to be abandoned.

This revision now requires changes to proceed.Feb 15 2022, 10:09 PM

We have an agreement not to use plural names in table names

Huh interesting, at least with the SQLite client DB all the table names are plural (eg messages, threads, drafts, etc).

I can abandon this if tunnelbroker_message is the correct name

We have an agreement not to use plural names in table names, that's why in our codebase we are using not plural names.

The initial precedent established in both MySQL and SQLite was to name tables using plural nouns. Frankly I find it disingenuous to indicate that there was a general "agreement" here when in fact you and @karol-bisztyga broke with precedent (eg. broke the existing agreement) and came up with your own.

Next time please do the research to determine if there is an existing convention, instead of just making up a new convention and trying to force everybody to use it.

We have an agreement not to use plural names in table names

Huh interesting, at least with the SQLite client DB all the table names are plural (eg messages, threads, drafts, etc).

I can abandon this if tunnelbroker_message is the correct name

@atul, please don't abandon diffs like this when you have a relevant reply like above. Can you please reopen the diff and re-request review so the discussion can continue?

In D3213#86933, @ashoat wrote:

We have an agreement not to use plural names in table names, that's why in our codebase we are using not plural names.

The initial precedent established in both MySQL and SQLite was to name tables using plural nouns. Frankly I find it disingenuous to indicate that there was a general "agreement" here when in fact you and @karol-bisztyga broke with precedent (eg. broke the existing agreement) and came up with your own.

Next time please do the research to determine if there is an existing convention, instead of just making up a new convention and trying to force everybody to use it.

We have an agreement not to use plural names in table names

Huh interesting, at least with the SQLite client DB all the table names are plural (eg messages, threads, drafts, etc).

I can abandon this if tunnelbroker_message is the correct name

@atul, please don't abandon diffs like this when you have a relevant reply like above. Can you please reopen the diff and re-request review so the discussion can continue?

The first table introduction was plural, then we are going to follow the existing code base with non-plural, so it was changed wise-versa.
I think we have some mess here and maybe in some other parts of the code.
As the codebase grows plus additional stacks added (Rust etc.). I think we need some single point of the code conventions. We have only tiny one for C++ right now.
In the past, I have used the "Code rules" in our projects. Here is an example from the past projects, it's outdated and abandoned, but maybe help as an example.

@geekbrother: in my opinion, trying to keep a single "repository of truth" for every single convention in a codebase never works. Either the "repository of truth" gets too large, or (more likely) people forget to update it.

Instead, you as a developer need to get better at searching for precedent. Before putting up a diff, you should look through your diff for any and all subjective decisions, and then think about where a precedent for that decision may have already been made. Checking precedent in MySQL and SQLite table naming should've been obvious given you were naming database tables.

This revision now requires changes to proceed.Feb 22 2022, 12:42 AM
atul requested review of this revision.Feb 22 2022, 9:47 AM
In D3213#86976, @ashoat wrote:

@geekbrother: in my opinion, trying to keep a single "repository of truth" for every single convention in a codebase never works. Either the "repository of truth" gets too large, or (more likely) people forget to update it.

Instead, you as a developer need to get better at searching for precedent. Before putting up a diff, you should look through your diff for any and all subjective decisions, and then think about where a precedent for that decision may have already been made. Checking precedent in MySQL and SQLite table naming should've been obvious given you were naming database tables.

That makes sense, but at the moment we have all the table names in all services as non-plural. This diff changes only one table name. If we want to change the table names to plural we need to introduce the new diff which changes all the table names and makes a migration on the DynamoDB side too: create or rename tables.

The reason why this diff was created is solved, I think we need to abandon it in favor to the new one which updates all table names.

atul planned changes to this revision.Feb 23 2022, 4:05 PM

The reason why this diff was created is solved, I think we need to abandon it in favor to the new one which updates all table names.

That makes sense. I'll plan changes on this diff and bring it back with all the table names changed.

We'll have to land this diff and update the corresponding names in DynamoDB quickly so people don't have a broken dev environment.

@geekbrother do you want to take this one? it's been a while and you're infinitely more familiar with tunnelbroker than I am

max edited reviewers, added: atul; removed: max.
In D3213#110138, @atul wrote:

@geekbrother do you want to take this one? it's been a while and you're infinitely more familiar with tunnelbroker than I am

@atul I commandeer this task. That's make sense as I have a more context on this.

Following our code convention about database names, fix all of dynamoDB table names to plural.

max edited the test plan for this revision. (Show Details)
max added a reviewer: tomek.
tomek requested changes to this revision.May 9 2022, 7:04 AM

I'm slightly confused here. In other places, e.g. backup service we still use singular names. Do the names that are changed here reflect what we already have on cloud? Do we need to change something else e.g. terraform? It looks like tunnelbroker tables aren't included there https://github.com/CommE2E/comm/blob/master/services/terraform/dynamodb.tf.

This revision now requires changes to proceed.May 9 2022, 7:04 AM

We have plural names in MySQL. But I think the original point of this diff was to fix the bug that @atul pointed out. Is that bug still active? And if so, does this diff still fix that bug?

In D3213#111109, @palys-swm wrote:

I'm slightly confused here. In other places, e.g. backup service we still use singular names. Do the names that are changed here reflect what we already have on cloud? Do we need to change something else e.g. terraform? It looks like tunnelbroker tables aren't included there https://github.com/CommE2E/comm/blob/master/services/terraform/dynamodb.tf.

Tunnelbroker tables for Terraform is on the D3965 diff.

We have plural names in MySQL. But I think the original point of this diff was to fix the bug that @atul pointed out. Is that bug still active? And if so, does this diff still fix that bug?

The bug was fixed because we change the table name in AWS, but it was a temporary fix until all table names will be refactored to plural to be the same with MySQL tables.

tomek requested changes to this revision.May 12 2022, 3:21 AM

I'm still confused but it sounds like the current code works ok and after this diff the names will be incorrect, right @geekbrother? If that's the case we need to remember about updating the names on the cloud after landing this. Is there a way to do it automatically, or are we planning to do it manually?

This revision now requires changes to proceed.May 12 2022, 3:21 AM
In D3213#112555, @palys-swm wrote:

I'm still confused but it sounds like the current code works ok and after this diff the names will be incorrect, right @geekbrother? If that's the case we need to remember about updating the names on the cloud after landing this. Is there a way to do it automatically, or are we planning to do it manually?

The current code works, this change is to make the codebase and names consistent.
Yes, we need to update the names in the AWS cloud after landing. It's mentioned in the diff description: Landing procedure section.

In D3213#112562, @geekbrother wrote:
In D3213#112555, @palys-swm wrote:

I'm still confused but it sounds like the current code works ok and after this diff the names will be incorrect, right @geekbrother? If that's the case we need to remember about updating the names on the cloud after landing this. Is there a way to do it automatically, or are we planning to do it manually?

The current code works, this change is to make the codebase and names consistent.
Yes, we need to update the names in the AWS cloud after landing. It's mentioned in the diff description: Landing procedure section.

Thanks for clarifying!

This revision is now accepted and ready to land.May 12 2022, 4:05 AM