Page MenuHomePhabricator
Differential D3512

[services] Backup - Add server reactor base classes - read reactor
ClosedPublic
Actions

Authored by • karol on Mar 24 2022, 1:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 10:49 PM
Unknown Object (File)
Wed, Nov 13, 4:00 AM
Unknown Object (File)
Sun, Oct 27, 4:14 AM
Unknown Object (File)
Oct 20 2024, 11:04 AM
Unknown Object (File)
Oct 13 2024, 7:34 PM
Unknown Object (File)
Oct 13 2024, 7:34 PM
Unknown Object (File)
Oct 13 2024, 7:34 PM
Unknown Object (File)
Oct 13 2024, 7:34 PM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
• max
varun
• jim
Commits
rCOMM67ccbc411b12: [services] Backup - Add server reactor base classes - read reactor
Summary

Depends on D3464

This class is moved and renamed for a better folder structure.
As we need to add client reactors in order to be able to connect to the blob service, we need to distinguish server reactors from client reactors.

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:49 AM
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 24 2022, 1:49 AM
Harbormaster completed remote builds in B7568: Diff 10639.Mar 24 2022, 1:54 AM
• karol requested review of this revision.Mar 24 2022, 1:54 AM
• karol planned changes to this revision.Mar 24 2022, 2:10 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, • jim.
• karol added a parent revision: D3464: [services] Backup - Add server reactor base classes - bidi reactor.
• karol updated this revision to Diff 10651.Mar 24 2022, 2:11 AM
• karol edited the summary of this revision. (Show Details)

update

• karol added a child revision: D3513: [services] Backup - Add server reactor base classes - write reactor.Mar 24 2022, 2:29 AM
Harbormaster completed remote builds in B7580: Diff 10651.Mar 24 2022, 2:36 AM
• karol planned changes to this revision.Mar 24 2022, 2:44 AM
• karol updated this revision to Diff 10658.Mar 24 2022, 2:45 AM

fix build

• karol retitled this revision from [services] Backup - Add server reactor base classes - read reactor to [draft] [services] Backup - Add server reactor base classes - read reactor.Mar 24 2022, 2:47 AM
• karol edited the summary of this revision. (Show Details)Mar 24 2022, 2:49 AM
• karol edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B7587: Diff 10658.Mar 24 2022, 3:06 AM
• karol planned changes to this revision.Mar 24 2022, 3:13 AM
• karol updated this revision to Diff 10662.Mar 24 2022, 3:14 AM

rebase

Harbormaster completed remote builds in B7591: Diff 10662.Mar 24 2022, 3:22 AM
tomek accepted this revision.Mar 24 2022, 5:27 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerReadReactorBase.h
21

Do we need to make this atomic?

This revision is now accepted and ready to land.Mar 24 2022, 5:27 AM
ashoat added inline comments.Mar 24 2022, 5:34 PM
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerReadReactorBase.h
30

It's strange that clang-format omits the space between () and {} here, but includes it when it is multiline. Are you sure scripts/get_clang_paths.js is covering this directory?

• karol added a comment.Mar 25 2022, 2:32 AM

About the threading problem: Let's wait for an answer on the SO. If there's none, we may want to research it on our own. I created a task for this https://linear.app/comm/issue/ENG-914/take-care-of-the-threading-problem-in-the-grpc-async-api

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerReadReactorBase.h
21

I'm not sure about how to approach changing the reactors' state. In the grpc examples, they do not synchronize the state manually. On the other hand, there may be a problem that you pointed out about not flushing the updated state to the main memory.

I saw multiple similar comments from you about this. As this is a bit confusing, I took some time and prepared a question on StackOverflow about this. I hope we get a response from the grpc authors.

30

Yes, clang takes care of this directory.

BTW, there are some problems in the scripts/get_clang_paths.js. I'm going to fix this.

• karol requested review of this revision.Mar 25 2022, 2:38 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/base-reactors/ServerReadReactorBase.h
30

I created a task for this https://linear.app/comm/issue/ENG-915/services-fix-clang-paths

• karol retitled this revision from [draft] [services] Backup - Add server reactor base classes - read reactor to [services] Backup - Add server reactor base classes - read reactor.Mar 25 2022, 2:39 AM
tomek accepted this revision.Mar 28 2022, 9:30 AM
This revision is now accepted and ready to land.Mar 28 2022, 9:30 AM
Closed by commit rCOMM67ccbc411b12: [services] Backup - Add server reactor base classes - read reactor (authored by • karol). · Explain WhyMar 31 2022, 7:18 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM67ccbc411b12: [services] Backup - Add server reactor base classes - read reactor.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
9 lines
Reactors/
server/
base-reactors/
ServerReadReactorBase.h
ReadReactorBase.h
34 lines

Diff 10662

View Options

services/backup/docker-server/contents/server/src/BackupServiceImpl.cpp

Loading...
View Options

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

Loading...
View Options

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

Loading...