Page MenuHomePhabricator

[services] Blob - Add server base reactor classes - bidi reactor
ClosedPublic

Authored by karol on Mar 21 2022, 2:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:35 PM
Unknown Object (File)
Fri, Dec 20, 7:31 PM

Details

Summary

Depends on D3468

Add base reactor classes for the server to the blob service so it can use the async API.

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

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

karol retitled this revision from [services] Blob - Add server base reactor classes to [draft] [services] Blob - Add server base reactor classes.Mar 21 2022, 3:18 AM
karol retitled this revision from [draft] [services] Blob - Add server base reactor classes to [draft] [services] Blob - Add server base reactor classes - bidi reactor.

Accepting as this is just a copy and we're going to reduce the duplication between services soon

services/blob/CMakeLists.txt
55–56 ↗(On Diff #10655)

Should we also update clang-format paths? (probably a lot of other diffs have the same issue)

This revision is now accepted and ready to land.Mar 24 2022, 8:23 AM

Accepting as this is just a copy and we're going to reduce the duplication between services soon

Would be great if this was tracked in a task on Linear!

services/blob/CMakeLists.txt
55–56 ↗(On Diff #10655)

I was wondering about this, but I think scripts/get_clang_paths.js might be recursive and it already lists services/blob/docker-server/contents/server/src

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/blob/CMakeLists.txt
55–56 ↗(On Diff #10655)

it goes recursively

karol retitled this revision from [draft] [services] Blob - Add server base reactor classes - bidi reactor to [services] Blob - Add server base reactor classes - bidi reactor.Mar 25 2022, 8:28 AM
In D3469#95752, @ashoat wrote:

Accepting as this is just a copy and we're going to reduce the duplication between services soon

Would be great if this was tracked in a task on Linear!

https://linear.app/comm/issue/ENG-919/make-sure-reactor-classes-are-not-copy-pasted-across-multiple-services

This revision is now accepted and ready to land.Mar 28 2022, 9:02 AM
services/blob/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
57 ↗(On Diff #10718)

Feels sketchy to see delete this... is this really the preferred pattern?

services/blob/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
57 ↗(On Diff #10718)

I'm almost sure we talked about this but cannot find where it is... gRPC authors did this and it is also said that delete this is ok if you don't use that object after doing this.

services/blob/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
57 ↗(On Diff #10718)
services/blob/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
57 ↗(On Diff #10718)

Thanks for linking! If this comes up so often, it seems like something that any reader of this code will ask. Could be good to include a code comment linking to the discussion in each place we use delete this. Or just link to a single page on Notion that explains it

services/blob/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
57 ↗(On Diff #10718)