Page MenuHomePhabricator

[draft] [services] Backup - Add Logs
AbandonedPublic

Authored by michal on Jul 27 2022, 6:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jun 24, 11:19 PM
Unknown Object (File)
Thu, Jun 20, 4:44 AM
Unknown Object (File)
Wed, Jun 19, 10:23 AM
Unknown Object (File)
Wed, Jun 19, 10:23 AM
Unknown Object (File)
Wed, Jun 19, 10:18 AM
Unknown Object (File)
Thu, Jun 13, 1:14 PM
Unknown Object (File)
May 10 2024, 12:01 PM
Unknown Object (File)
May 7 2024, 2:11 AM

Details

Summary

Depends on D4652

Adding Logs to the backup service. Putting this up as a draft since I'm not 100% sure about this. Curious about your opinions.

Test Plan
cd services
yarn run-backup-service-in-sandbox

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

karol retitled this revision from [services] Backup - Add Logs to [draft] [services] Backup - Add Logs.Jul 27 2022, 6:35 AM
karol edited the summary of this revision. (Show Details)
karol edited the test plan for this revision. (Show Details)
tomek requested changes to this revision.Jul 28 2022, 4:58 AM

Looking at this diff, the idea from https://phab.comm.dev/D4652#inline-29573 sounds really beneficial.

services/backup/src/DatabaseManager.cpp
24

Do you think we can safely log user id? @ashoat what do you think?

services/backup/src/Reactors/server/CreateNewBackupReactor.cpp
54

Is logging this safe? Can it be used in some attacks?

services/backup/src/Reactors/server/PullBackupReactor.cpp
85–88

Can we avoid this confusing notation and simply write values next to their descriptions (in all the places)?

119

Not related to this diff, but are we still affected by this issue?

services/backup/src/Reactors/server/SendLogReactor.cpp
58

Is writing this as an int really useful?

129–131

We're computing the size twice. Can we avoid it?

This revision now requires changes to proceed.Jul 28 2022, 4:58 AM
services/backup/src/DatabaseManager.cpp
24

Let's avoid that, see here https://phab.comm.dev/D4652#133661

I think this diff should be canceled because the Backup service going to Rust now. I can commandeer and close it if @tomek doesn't mind.

michal abandoned this revision.
michal added a reviewer: karol.

Old diff, that isn't applicable to the current backup service.