Page MenuHomePhabricator
Differential D3515

[services] Backup - Add client reactor base classes - write reactor
ClosedPublic
Actions

Authored by • karol on Mar 24 2022, 1:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 30, 3:07 PM
Unknown Object (File)
Sun, Jun 30, 3:07 PM
Unknown Object (File)
Sun, Jun 30, 3:07 PM
Unknown Object (File)
Sun, Jun 30, 3:06 PM
Unknown Object (File)
Sun, Jun 30, 3:03 PM
Unknown Object (File)
Sun, Jun 30, 12:45 PM
Unknown Object (File)
Sun, Jun 23, 5:21 PM
Unknown Object (File)
Sun, Jun 23, 1:45 AM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
• jim
• max
tomek
varun
Commits
rCOMM3ffc1665dda6: [services] Backup - Add client reactor base classes - write reactor
Summary

Depends on D3514

Adding client base classes for reactors - write reactor

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• karol created this revision.Mar 24 2022, 1:51 AM
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 24 2022, 1:51 AM
Harbormaster completed remote builds in B7571: Diff 10642.Mar 24 2022, 2:05 AM
• karol requested review of this revision.Mar 24 2022, 2:05 AM
• karol edited the summary of this revision. (Show Details)Mar 24 2022, 2:14 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: • jim, • max, tomek, varun.
• karol edited the summary of this revision. (Show Details)
• karol added a parent revision: D3514: [services] Backup - Add client reactor base classes - read reactor.
• karol added a child revision: D3466: [services] Backup - Add blob client.Mar 24 2022, 2:17 AM
• karol retitled this revision from [services] Backup - Add client reactor base classes - write reactor to [draft] [services] Backup - Add client reactor base classes - write reactor.Mar 24 2022, 2:47 AM
tomek accepted this revision.Mar 24 2022, 6:20 AM

Most of these classes are really similar. Is there a way to share more logic?

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientWriteReactorBase.h
38–41 ↗(On Diff #10642)

Should we call this earlier?

This revision is now accepted and ready to land.Mar 24 2022, 6:20 AM
ashoat added a comment.Mar 24 2022, 5:40 PM

Most of these classes are really similar. Is there a way to share more logic?

Yeah, would love to see less copy-paste. Happy for this to be covered in a follow-up task, though. (Let's make sure the task explicitly talks about deduping reactor code.)

• karol added a comment.Mar 25 2022, 7:14 AM
In D3515#95613, @palys-swm wrote:

Most of these classes are really similar. Is there a way to share more logic?

I had a thought like this, definitely would go for this separately.

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientWriteReactorBase.h
38–41 ↗(On Diff #10642)

No, please see my response in https://phabricator.ashoat.com/D3465#95858

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

I created a task for this:
https://linear.app/comm/issue/ENG-916/consider-pulling-more-mutual-code-from-the-async-reactors

Separately: 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.

• karol retitled this revision from [draft] [services] Backup - Add client reactor base classes - write reactor to [services] Backup - Add client reactor base classes - write reactor.Mar 25 2022, 7:17 AM
tomek accepted this revision.Mar 28 2022, 9:48 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientWriteReactorBase.h
60–61 ↗(On Diff #10642)

Can we include more context in this log?

This revision is now accepted and ready to land.Mar 28 2022, 9:48 AM
• karol added inline comments.Mar 29 2022, 12:27 AM
services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientWriteReactorBase.h
60–61 ↗(On Diff #10642)

it will be removed

• karol updated this revision to Diff 10749.Mar 29 2022, 12:50 AM

remove log

Harbormaster completed remote builds in B7654: Diff 10749.Mar 29 2022, 12:58 AM
Closed by commit rCOMM3ffc1665dda6: [services] Backup - Add client reactor base classes - write 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: rCOMM3ffc1665dda6: [services] Backup - Add client reactor base classes - write reactor.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
client/
base-reactors/
78 lines

Diff 10892

View Options

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientWriteReactorBase.h

Loading...