Page MenuHomePhabricator
Differential D3087

[services] Backup - Database - add Backup Item
ClosedPublic
Actions

Authored by • karol on Feb 3 2022, 6:51 AM.
Tags
None
Referenced Files
F3535600: D3087.id9367.diff
Wed, Dec 25, 4:25 PM
F3530931: D3087.diff
Wed, Dec 25, 5:20 AM
Unknown Object (File)
Fri, Dec 20, 6:36 PM
Unknown Object (File)
Fri, Dec 20, 6:36 PM
Unknown Object (File)
Fri, Dec 20, 6:35 PM
Unknown Object (File)
Fri, Dec 20, 6:35 PM
Unknown Object (File)
Fri, Dec 20, 6:35 PM
Unknown Object (File)
Fri, Dec 20, 6:35 PM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
• max
varun
ashoat
• jim
Commits
rCOMM2d9294c89284: [services] Backup - Database - add Backup Item
Summary

C++ representation of the backup database entity

Depends on D3084

Test Plan

-

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• karol created this revision.Feb 3 2022, 6:51 AM
Herald added subscribers: atul, • adrian, tomek, ashoat. · View Herald TranscriptFeb 3 2022, 6:51 AM
• karol published this revision for review.Feb 3 2022, 7:12 AM
• karol edited the summary of this revision. (Show Details)
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: tomek, • max, varun.
Harbormaster completed remote builds in B6485: Diff 9217.Feb 3 2022, 8:55 AM
• karol edited the summary of this revision. (Show Details)Feb 8 2022, 1:25 AM
• karol added a parent revision: D3084: [services] Backup - Database - add Log Item.
• karol planned changes to this revision.Feb 8 2022, 1:43 AM
• karol updated this revision to Diff 9367.Feb 8 2022, 1:45 AM

update

• karol edited the summary of this revision. (Show Details)Feb 8 2022, 1:46 AM
Harbormaster failed remote builds in B6611: Diff 9367!Feb 8 2022, 2:06 AM
tomek accepted this revision.Feb 8 2022, 8:43 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h
19 ↗(On Diff #9367)

Interesting... shouldn't #include <vector> be necessary?

24 ↗(On Diff #9367)

Can this be const?

This revision is now accepted and ready to land.Feb 8 2022, 8:43 AM
Herald added a reviewer: ashoat. · View Herald TranscriptFeb 8 2022, 8:43 AM
Herald added a subscriber: • benschac. · View Herald Transcript
This revision now requires review to proceed.Feb 8 2022, 8:43 AM
ashoat accepted this revision.Feb 8 2022, 8:41 PM
ashoat added a reviewer: • jim.
ashoat added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h
19 ↗(On Diff #9367)

Please add this. Often C++ seems to let this sort of thing go, but I wish it didn't

This revision is now accepted and ready to land.Feb 8 2022, 8:41 PM
• karol updated this revision to Diff 9437.Feb 9 2022, 3:59 AM
• karol edited the summary of this revision. (Show Details)

include vector

Harbormaster completed remote builds in B6666: Diff 9437.Feb 9 2022, 4:04 AM
• karol planned changes to this revision.Feb 9 2022, 4:18 AM
• karol added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h
18 ↗(On Diff #9437)

this should be merged with the recoveryData

19 ↗(On Diff #9367)

right, thanks

24 ↗(On Diff #9367)

This can be const but I think we should refactor all services
https://linear.app/comm/issue/ENG-707/convert-table-names-to-const

• karol updated this revision to Diff 9440.Feb 9 2022, 4:21 AM

encryptedBackupKey merged into recoveryData

This revision is now accepted and ready to land.Feb 9 2022, 4:21 AM
Harbormaster completed remote builds in B6669: Diff 9440.Feb 9 2022, 4:26 AM
• karol edited the summary of this revision. (Show Details)Feb 10 2022, 6:33 AM
• karol added a child revision: D3158: [services] Backup - Implementation structure.
• karol updated this revision to Diff 9654.Feb 15 2022, 4:30 AM

update

• karol requested review of this revision.Feb 15 2022, 4:30 AM
Harbormaster completed remote builds in B6815: Diff 9654.Feb 15 2022, 4:35 AM
ashoat accepted this revision.Feb 15 2022, 7:36 PM
ashoat added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
57–59 ↗(On Diff #9367)

Did you purposely remove the compactionHolder validation here? Not sure why

This revision is now accepted and ready to land.Feb 15 2022, 7:36 PM
• karol added inline comments.Feb 16 2022, 12:24 AM
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
57–59 ↗(On Diff #9367)

Yes. This is because I added the index for created as we discussed in https://phabricator.ashoat.com/D3160. In this index, we will have the following fields projected:

userID
backupID
created
recoveryData

There's no point in projecting compaction data for RecoverBackupKey.
Therefore we'll encounter a situation when the compaction data will not be provided and it will not be an error.

tomek accepted this revision.Feb 16 2022, 3:02 AM
ashoat added a comment.Feb 21 2022, 8:22 PM

Makes sense. Would be good to add a code comment clarifying this, see inline

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h
12 ↗(On Diff #9654)

Might be good to add a code comment here clarifying that this class is used for representing two things: the rows in the main table, and also the rows in the secondary index

• karol updated this revision to Diff 9800.Feb 22 2022, 1:15 AM

format

Harbormaster completed remote builds in B6946: Diff 9800.Feb 22 2022, 1:20 AM
Closed by commit rCOMM2d9294c89284: [services] Backup - Database - add Backup Item (authored by • karol). · Explain WhyMar 4 2022, 3:56 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM2d9294c89284: [services] Backup - Database - add Backup Item.
ashoat added inline comments.Mar 7 2022, 12:29 PM
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h
32

The requested code comment was not added here!! This is very concerning to see... I want to be able to accept diffs with comments, but actions like this where you ignore my comments make it really hard for me to do that.

@karol-bisztyga: in the future, would you prefer that I request changes any time I have any small comments?

ashoat mentioned this in D3160: [services] Backup - Add DB operations for Backup.Mar 7 2022, 12:33 PM
• karol mentioned this in D3420: [services] Add comments about indexes.Mar 14 2022, 8:51 AM

Hey, sorry about this. I think we can keep the current pattern of accepting + comments, I address everything most of the time, not sure why I missed these.

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h
12 ↗(On Diff #9654)

OK, addressed in https://phabricator.ashoat.com/D3420

• karol mentioned this in rCOMM82073b570fb5: [services] Add comments about indexes.Mar 22 2022, 1:40 AM

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
2 lines
DatabaseEntities/
58 lines
111 lines
1 line

Diff 10058

View Options

services/backup/docker-server/contents/server/src/Constants.h

Loading...
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h

Loading...
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp

Loading...
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h

Loading...