Page MenuHomePhabricator
Differential D3464

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

Authored by • karol on Mar 21 2022, 2:38 AM.
Tags
None
Referenced Files
F3355822: D3464.id10722.diff
Sat, Nov 23, 3:41 PM
F3355738: D3464.id10657.diff
Sat, Nov 23, 3:16 PM
F3355719: D3464.id10660.diff
Sat, Nov 23, 3:09 PM
F3355712: D3464.id10649.diff
Sat, Nov 23, 3:08 PM
F3355685: D3464.id10650.diff
Sat, Nov 23, 3:00 PM
F3355654: D3464.id10814.diff
Sat, Nov 23, 2:55 PM
F3355396: D3464.id10661.diff
Sat, Nov 23, 2:27 PM
F3355323: D3464.id10540.diff
Sat, Nov 23, 2:18 PM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

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

Depends on D3463

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 21 2022, 2:38 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Mar 21 2022, 2:49 AM
Harbormaster failed remote builds in B7485: Diff 10540!
• karol updated this revision to Diff 10550.Mar 21 2022, 3:05 AM

update

• karol edited the summary of this revision. (Show Details)Mar 21 2022, 3:06 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: varun, • max, tomek, • jim.
• karol added a child revision: D3465: [services] Backup - Add client reactor base classes - bidi reactor.
• karol edited the summary of this revision. (Show Details)
• karol added a parent revision: D3463: [services] Backup - Add random string to tools.
• karol retitled this revision from [services] Backup - Add server reactor base classes to [draft] [services] Backup - Add server reactor base classes.Mar 21 2022, 3:09 AM
Harbormaster completed remote builds in B7494: Diff 10550.Mar 21 2022, 3:24 AM
• karol requested review of this revision.Mar 21 2022, 3:24 AM
tomek requested changes to this revision.Mar 22 2022, 5:36 AM

A lot is happening in this diff: 1. ServerBidiReactorBase is introduced 2. ServerWriteReactorBase and ServerReadReactorBase are modified and 3. new reactors are used in BackupServiceImpl. Could you split this diff so that it's easier to review it?
Also, in 1. it looks like some parts of the code was moved. When moving the code, we should prefer to not modify it. We sometimes can modify it to make it compile, but there should be an explanation what we're doing.

This revision now requires changes to proceed.Mar 22 2022, 5:36 AM
• karol updated this revision to Diff 10649.Mar 24 2022, 1:55 AM

update

• karol retitled this revision from [draft] [services] Backup - Add server reactor base classes to [draft] [services] Backup - Add server reactor base classes - bidi reactor.Mar 24 2022, 1:55 AM
• karol planned changes to this revision.Mar 24 2022, 2:10 AM
• karol added a child revision: D3512: [services] Backup - Add server reactor base classes - read reactor.
• karol edited the summary of this revision. (Show Details)
• karol updated this revision to Diff 10650.Mar 24 2022, 2:11 AM

update

Harbormaster failed remote builds in B7578: Diff 10649!Mar 24 2022, 2:21 AM
Harbormaster failed remote builds in B7579: Diff 10650!Mar 24 2022, 2:24 AM
• karol planned changes to this revision.Mar 24 2022, 2:44 AM
• karol updated this revision to Diff 10657.Mar 24 2022, 2:45 AM

fix build

Harbormaster failed remote builds in B7586: Diff 10657!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)
• karol planned changes to this revision.Mar 24 2022, 2:52 AM

CI

• karol updated this revision to Diff 10660.Mar 24 2022, 2:53 AM

fix build

Harbormaster failed remote builds in B7589: Diff 10660!Mar 24 2022, 2:55 AM
• karol planned changes to this revision.Mar 24 2022, 3:12 AM
• karol updated this revision to Diff 10661.Mar 24 2022, 3:13 AM

fix build

Harbormaster completed remote builds in B7590: Diff 10661.Mar 24 2022, 3:19 AM
tomek requested changes to this revision.Mar 24 2022, 5:19 AM

Thanks for splitting the diff - it's a lot better now.

Is there a way to avoid sendLastResponse flag? It makes it rally hard to be sure that terminate works correctly. Can we e.g. pass it as a parameter to some functions?

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
65 ↗(On Diff #10661)

Not sure about the correctness of the logic here: so when !ok and sendLastResponse we're going to StartWriteAndFinish in terminate which sounds like a mistake. I feel like we don't need sendLastResponse flag - there should be some other ways of achieving the desired result without it, but I don't have a good understanding of the whole logic.

This revision now requires changes to proceed.Mar 24 2022, 5:19 AM
• karol requested review of this revision.Mar 24 2022, 7:44 AM

back to your queue, curious for your thoughts.

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
65 ↗(On Diff #10661)

I'm not sure if that's possible, but I may be missing something.
Here's what we need:
Once we return a non-null status from the handleRequest function, we want to have the ability to send one final message to the client before terminating the connection. A case for that was letting the client know that the data already exists in the blob so we want to abandon sending it again (D3521).
What happens is, the response is being reset this->response = Response(); before this call this->handleRequest (this->request, &this->response);. I think it would be sufficient to recognize whether there were any changes made to the response field. If so, we'd send the last response, if not, we would not (before the connection terminates).
Setting a field like sendLastResponse is not perfect (a little bit unintuitive) but I couldn't figure out a better solution. We could make the response a unique ptr for example but then it would be a lot less handy to use inside of the handleRqeuest function.

tomek requested changes to this revision.Mar 25 2022, 7:57 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
65 ↗(On Diff #10661)

Looking at D3521 it seems like we can handle this case by introducing a new status. The status could consist of two fields, e.g. sendLastResponse and grpcStatus and if sendLastResponse is true, then we would call StartWriteAndFinish in terminate. Could that solution work?

This revision now requires changes to proceed.Mar 25 2022, 7:57 AM
• karol added inline comments.Mar 28 2022, 1:27 AM
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
65 ↗(On Diff #10661)

This solution will probably work alright, it's worth mentioning though that it has some inconsistency included (we don't return a grpc::Status like everywhere else).

All in all, I don't see a better approach to this at this point. Thanks for the idea, I'm going to implement this.

• karol updated this revision to Diff 10713.Mar 28 2022, 3:43 AM

update

Harbormaster failed remote builds in B7625: Diff 10713!Mar 28 2022, 3:46 AM
• karol planned changes to this revision.Mar 28 2022, 4:21 AM

fix build

• karol updated this revision to Diff 10722.Mar 28 2022, 4:24 AM

fix build

Harbormaster completed remote builds in B7634: Diff 10722.Mar 28 2022, 4:30 AM
tomek requested changes to this revision.Mar 28 2022, 5:16 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
63 ↗(On Diff #10722)

It doesn't look like we need to keep the status in this

75 ↗(On Diff #10722)

We set this value but never check it - do we really need it? Maybe we can do something similar to sendLastResponse and avoid storing it in this?

This revision now requires changes to proceed.Mar 28 2022, 5:16 AM
• karol added inline comments.Mar 28 2022, 5:57 AM
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
63 ↗(On Diff #10722)

we may want to use it in the doneCallback

75 ↗(On Diff #10722)

This can also be used in doneCallback and was created for doing this. I make use of this in PutReactor

• karol requested review of this revision.Mar 28 2022, 5:58 AM
tomek accepted this revision.Mar 28 2022, 9:01 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
63 ↗(On Diff #10722)

Ah, ok, that makes sense

This revision is now accepted and ready to land.Mar 28 2022, 9:01 AM
• karol requested review of this revision.Mar 28 2022, 11:59 PM
• karol retitled this revision from [draft] [services] Backup - Add server reactor base classes - bidi reactor to [services] Backup - Add server reactor base classes - bidi reactor.

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.

tomek requested changes to this revision.Mar 29 2022, 8:13 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
74–100 ↗(On Diff #10722)

In OnReadDone when !ok we call terminate. Why in OnWriteDone we don't do the same?

This revision now requires changes to proceed.Mar 29 2022, 8:13 AM
• karol updated this revision to Diff 10814.Mar 30 2022, 5:38 AM

terminate when write fails

• karol added inline comments.Mar 30 2022, 5:40 AM
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
74–100 ↗(On Diff #10722)

Right, thanks

Harbormaster completed remote builds in B7712: Diff 10814.Mar 30 2022, 5:45 AM
tomek accepted this revision.Mar 31 2022, 7:00 AM
This revision is now accepted and ready to land.Mar 31 2022, 7:00 AM
Closed by commit rCOMMc756f4fc3751: [services] Backup - Add server reactor base classes - bidi reactor (authored by • karol). · Explain WhyMar 31 2022, 7:16 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMMc756f4fc3751: [services] Backup - Add server reactor base classes - bidi reactor.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
6 lines
src/
Reactors/
server/
base-reactors/
93 lines
75 lines
86 lines

Diff 10540

View Options

services/backup/docker-server/contents/server/CMakeLists.txt

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...