C++ representation of the backup database entity
Depends on D3084
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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 |
| 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 |
| 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 |
| 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. |
Comment Actions
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 |
| 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? | |
Comment Actions
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 |