Page MenuHomePhabricator
Differential D3517

[services] Backup - Add server reactor implementations - pull backup reactor
ClosedPublic
Actions

Authored by • karol on Mar 24 2022, 1:52 AM.
Tags
None
Referenced Files
F3254295: D3517.id10644.diff
Fri, Nov 15, 7:43 PM
F3253576: D3517.id10715.diff
Fri, Nov 15, 7:13 PM
F3252264: D3517.diff
Fri, Nov 15, 6:08 PM
Unknown Object (File)
Sun, Nov 10, 9:22 AM
Unknown Object (File)
Sat, Nov 9, 2:29 AM
Unknown Object (File)
Sat, Nov 9, 2:29 AM
Unknown Object (File)
Sat, Nov 9, 2:29 AM
Unknown Object (File)
Tue, Nov 5, 7:02 PM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
• max
tomek
varun
• jim
Commits
rCOMMcd537913f227: [services] Backup - Add server reactor implementations - pull backup reactor
Summary

Depends on D3466

Add implementation for the pull backup reactor

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Mar 24 2022, 1:52 AM
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 24 2022, 1:52 AM
Harbormaster completed remote builds in B7573: Diff 10644.Mar 24 2022, 2:11 AM
• karol requested review of this revision.Mar 24 2022, 2:11 AM
• karol edited the summary of this revision. (Show Details)Mar 24 2022, 2:19 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: • max, tomek, varun, • jim.
• karol added a parent revision: D3466: [services] Backup - Add blob client.
• karol added a child revision: D3467: [services] Backup - Add server reactor implementations - create new backup reactor.Mar 24 2022, 2:32 AM
• karol retitled this revision from [services] Backup - Add server reactor implementations - pull backup reactor to [draft] [services] Backup - Add server reactor implementations - pull backup reactor.Mar 24 2022, 2:47 AM
tomek accepted this revision.Mar 24 2022, 7:40 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
25–31

Is there a reason why this implementation is in the header file?

This revision is now accepted and ready to land.Mar 24 2022, 7:40 AM
ashoat added inline comments.Mar 24 2022, 5:52 PM
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
25–31

Agree that we should avoid implementations in .h files when possible. If it's addressed in a later diff, would be good to reference that here (or just keep move this stub to a .cpp file to avoid any confusion)

• karol requested review of this revision.Mar 25 2022, 7:58 AM

It is accepted as a draft so I removed the draft label and I'm requesting changes once again to keep the appropriate order of events.

services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
25–31

Yes, I was getting weird and frustrating compile errors with templates in the background. Maybe this code can be moved to a CPP file. I don't mind as long as it works. Can we do this separately? https://linear.app/comm/issue/ENG-917/try-to-move-reactors-implementations-to-cpp-files

• karol retitled this revision from [draft] [services] Backup - Add server reactor implementations - pull backup reactor to [services] Backup - Add server reactor implementations - pull backup reactor.Mar 25 2022, 7:58 AM
• karol planned changes to this revision.Mar 28 2022, 3:43 AM
• karol updated this revision to Diff 10715.Mar 28 2022, 3:45 AM

update

Harbormaster failed remote builds in B7627: Diff 10715!Mar 28 2022, 3:51 AM
• karol updated this revision to Diff 10725.Mar 28 2022, 4:32 AM

rebuild

• karol added a comment.Mar 28 2022, 4:32 AM

ops, forgot to plan changes, sorry

Harbormaster completed remote builds in B7637: Diff 10725.Mar 28 2022, 4:40 AM
tomek accepted this revision.Mar 28 2022, 9:51 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
16 ↗(On Diff #10725)

Why do we need a bidi reactor when pulling backup? Is it due to the authentication?

25–31

Sure, we can do that

This revision is now accepted and ready to land.Mar 28 2022, 9:51 AM
• karol added inline comments.Mar 29 2022, 12:34 AM
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
16 ↗(On Diff #10725)

May be an oversight. Even with auth, we needed a simple auth in this case. I think we can change this into a write reactor but maybe in a separate diff since it requires a proto file change.

• karol added inline comments.Mar 29 2022, 1:06 AM
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
16 ↗(On Diff #10725)

https://linear.app/comm/issue/ENG-936/refactor-pullbackup-to-the-writing-reactor

Closed by commit rCOMMcd537913f227: [services] Backup - Add server reactor implementations - pull backup reactor (authored by • karol). · Explain WhyMar 31 2022, 7:21 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMMcd537913f227: [services] Backup - Add server reactor implementations - pull backup reactor.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
server/
35 lines

Diff 10644

View Options

services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h

Loading...