Page MenuHomePhabricator

[services] Backup - Fix primary key logic
ClosedPublic

Authored by karol on Feb 11 2022, 3:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM
Unknown Object (File)
Tue, Apr 23, 5:30 PM

Details

Summary

Based on my research about the dynamo DB: https://phabricator.ashoat.com/D2950#81654

  • Every DynamoDB table has a unique primary key
  • A primary key [PKSK] must be composed of a partition key [PK], and can optionally have a sort key [SK].
  • A GetItem request returns one and only one item using its unique primary key [PKSK].
  • A Query does a fast lookup and must specify one and only one partition key [PK]. It can return multiple items.
  • A Scan evaluates every item in a table and may return a subset based on filter parameters. Scans are the correct choice in some circumstances but can be slow and costly if used incorrectly.

Depends on D3176

Test Plan

Backup service still builds

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, max, varun.
karol edited the summary of this revision. (Show Details)
tomek requested changes to this revision.Feb 11 2022, 7:43 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

It's ok to be explicit here and in PrimaryKey class, but we can consider making PrimaryKey an aggregate https://en.cppreference.com/w/cpp/language/aggregate_initialization. The consequences would be that here we could simply return {BackupItem::FIELD_USER_ID, BackupItem::FIELD_CREATED}.
The PrimaryKey can then be simply

struct PrimaryKey {
  const std::string partitionKey;
  std::unique_ptr<std::string> sortKey = nullptr;
};

What do you think about this idea?

services/backup/docker-server/contents/server/src/DatabaseManager.cpp
36–38 ↗(On Diff #9562)

We probably need to consider the whole primary key here, not only the partition

79–80 ↗(On Diff #9562)

Not sure, maybe we should explicitly use userID column here instead of the partition key. We want to find an item for a user and the coincidence that this is also a partition key should be an implementation detail at this point

146–147 ↗(On Diff #9562)

The same here. Do you think it makes sense to use user id column here explicitly?

This revision now requires changes to proceed.Feb 11 2022, 7:43 AM
ashoat requested changes to this revision.Feb 13 2022, 8:45 PM
ashoat added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

See feedback in D3160... I'm concerned that with this database design, it may not be possible to efficiently look up a backup based on BACKUP_ID, which is a requirement of the current architecture design

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

The consequences would be that here we could simply return {BackupItem::FIELD_USER_ID, BackupItem::FIELD_CREATED}.

That's not going to work. We'd have to manually call make_unique. So the current code lets you simply call a constructor with two strings. On the other hand, we could consider an empty string as a lack of sortKey . All these solutions are valid and more or less equal. I can change to something else although I cannot see any significant gain from doing so.

Ashoat - see my comments in D3160

services/backup/docker-server/contents/server/src/DatabaseManager.cpp
36–38 ↗(On Diff #9562)

That's a part of this solution, I just broke it down into two separate diffs, this one and D3178

79–80 ↗(On Diff #9562)

Yes, well, I'm not sure about this either... You're probably right, I'm gonna change this.

tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

That's not going to work. We'd have to manually call make_unique. So the current code lets you simply call a constructor with two strings. On the other hand, we could consider an empty string as a lack of sortKey . All these solutions are valid and more or less equal. I can change to something else although I cannot see any significant gain from doing so.

Ok, that makes sense. If we need to use make_unique the simplification isn't that significant - let's keep your approach

services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h
30 ↗(On Diff #9603)

What is the purpose of this statement? What happens when we don't have it?

ashoat added a reviewer: jim.
This revision is now accepted and ready to land.Feb 14 2022, 8:58 PM
karol added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h
30 ↗(On Diff #9603)

This is a constructor inheritance feature. It allows us to use constructors from the base class in the children classes.

This revision is now accepted and ready to land.Feb 15 2022, 7:41 PM
tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h
30 ↗(On Diff #9603)

Thanks!