Page MenuHomePhabricator
Differential D3470

[services] Blob - Add server reactor implementations - get reactor
ClosedPublic
Actions

Authored by • karol on Mar 21 2022, 2:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 20, 1:01 AM
Unknown Object (File)
Mon, Mar 17, 6:07 AM
Unknown Object (File)
Fri, Mar 14, 10:32 PM
Unknown Object (File)
Mon, Mar 10, 3:32 AM
Unknown Object (File)
Tue, Mar 4, 8:19 AM
Unknown Object (File)
Tue, Mar 4, 5:37 AM
Unknown Object (File)
Tue, Mar 4, 5:37 AM
Unknown Object (File)
Tue, Mar 4, 5:37 AM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
• max
tomek
varun
• jim
Commits
rCOMMd0b3a163e2fb: [services] Blob - Add server reactor implementations - get reactor
Summary

Depends on D3520

Add implementations for the server reactors - get 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 21 2022, 2:40 AM
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 21 2022, 2:40 AM
• karol edited the summary of this revision. (Show Details)Mar 21 2022, 3:13 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: • max, tomek, varun, • jim.
• karol added a parent revision: D3469: [services] Blob - Add server base reactor classes - bidi reactor.
• karol added a child revision: D3471: [services] Blob - Update service implementation.
Harbormaster completed remote builds in B7491: Diff 10546.Mar 21 2022, 3:14 AM
• karol requested review of this revision.Mar 21 2022, 3:14 AM
• karol retitled this revision from [services] Blob - Add server reactor implementations to [draft] [services] Blob - Add server reactor implementations.Mar 21 2022, 3:18 AM
ashoat added inline comments.Mar 21 2022, 8:11 AM
services/blob/docker-server/contents/server/src/Reactors/server/PutReactor.h
81–97

This logic can be cleaned up to reduce indentation. See note here

• karol edited the summary of this revision. (Show Details)Mar 24 2022, 2:23 AM
• karol added a parent revision: D3520: [services] Blob - Add server base reactor classes - write reactor.
• karol planned changes to this revision.Mar 24 2022, 2:27 AM
• karol retitled this revision from [draft] [services] Blob - Add server reactor implementations to [draft] [services] Blob - Add server reactor implementations - get reactor.
• karol edited the summary of this revision. (Show Details)
• karol added a child revision: D3521: [services] Blob - Add server reactor implementations - put reactor.
• karol updated this revision to Diff 10656.Mar 24 2022, 2:28 AM
• karol edited the summary of this revision. (Show Details)

update

Harbormaster completed remote builds in B7585: Diff 10656.Mar 24 2022, 2:55 AM
• karol planned changes to this revision.Mar 25 2022, 7:48 AM
• karol updated this revision to Diff 10689.Mar 25 2022, 7:50 AM

rebase to master

Harbormaster completed remote builds in B7613: Diff 10689.Mar 25 2022, 8:42 AM
tomek accepted this revision.Mar 25 2022, 9:01 AM
tomek added inline comments.
services/blob/src/Reactors/server/GetReactor.h
55 ↗(On Diff #10689)

Why do we add 1 here?

This revision is now accepted and ready to land.Mar 25 2022, 9:01 AM
• karol added inline comments.Mar 28 2022, 3:14 AM
services/blob/src/Reactors/server/GetReactor.h
55 ↗(On Diff #10689)

For now, I don't have an answer right away. This was introduced in D2672. I removed the + 1 and ran the blob test and it failed, saying that the retrieved string differed from the original one.

If you feel a strong need to know why exactly this is needed, I can check, but I'm willing to trust our previous reviews as well as tests results.

• karol requested review of this revision.Mar 28 2022, 3:49 AM
• karol retitled this revision from [draft] [services] Blob - Add server reactor implementations - get reactor to [services] Blob - Add server reactor implementations - get 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 30 2022, 4:19 AM
tomek added inline comments.
services/blob/src/Reactors/server/GetReactor.h
55 ↗(On Diff #10689)

It looks like Aws::IOStream is just a wrapper on std::basic_iostream where get method:

reads at most count-1 characters and stores them into character string pointed to by s until '\n' is found

So having +1 here makes sense.

Do you know if we can have \n in this file? If yes, then should we modify the logic so that reading doesn't stop on \n?

This revision now requires changes to proceed.Mar 30 2022, 4:19 AM
• karol requested review of this revision.Apr 1 2022, 3:11 AM
• karol added inline comments.
services/blob/src/Reactors/server/GetReactor.h
55 ↗(On Diff #10689)

Nice catch with the \n!
The objects are properly stored with the newline signs but reading seems problematic indeed.
Thank you for catching this, I'm going to fix this!

55 ↗(On Diff #10689)

Ok So I decided to fix this in a separate diff since this issue applies not only here but also in
https://github.com/CommE2E/comm/blob/889b51283a2b207d86d2cb6f2a3c933974ea2768/services/blob/src/AwsS3Bucket.cpp
https://github.com/CommE2E/comm/blob/889b51283a2b207d86d2cb6f2a3c933974ea2768/services/blob/src/AwsS3Bucket.cpp#L157

D3594

• karol mentioned this in D3594: [services] Blob - Fix reading data from S3.Apr 1 2022, 3:17 AM
tomek accepted this revision.Apr 1 2022, 4:11 AM
This revision is now accepted and ready to land.Apr 1 2022, 4:11 AM
Closed by commit rCOMMd0b3a163e2fb: [services] Blob - Add server reactor implementations - get reactor (authored by • karol). · Explain WhyApr 4 2022, 2:39 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMMd0b3a163e2fb: [services] Blob - Add server reactor implementations - get reactor.
• karol mentioned this in rCOMM656be3bc4746: [services] Blob - Fix reading data from S3.Apr 13 2022, 4:58 AM

Revision Contents
Changeset List

PathSize
services/
blob/
docker-server/
contents/
server/
src/
Reactors/
server/
86 lines
103 lines

Diff 10546

View Options

services/blob/docker-server/contents/server/src/Reactors/server/GetReactor.h

::-webkit-scrollbar { ::-webkit-scrollbar {
width: 15px; width: 15px;
} }
::-webkit-scrollbar-track-piece { ::-webkit-scrollbar-track-piece {
background-color: var(--shades-black-80); background-color: var(--shades-black-75);
ginsuAuthorUnsubmitted
Done
Inline Actions

This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar

ginsu: This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar
} }
::-webkit-scrollbar-thumb { ::-webkit-scrollbar-thumb {
background-color: var(--shades-black-60); background-color: var(--shades-black-50);
border: 3px var(--shades-black-80) solid; border: 3px var(--shades-black-75) solid;
border-radius: 10px; border-radius: 10px;
} }
View Options

services/blob/docker-server/contents/server/src/Reactors/server/PutReactor.h

::-webkit-scrollbar { ::-webkit-scrollbar {
width: 15px; width: 15px;
} }
::-webkit-scrollbar-track-piece { ::-webkit-scrollbar-track-piece {
background-color: var(--shades-black-80); background-color: var(--shades-black-75);
ginsuAuthorUnsubmitted
Done
Inline Actions

This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar

ginsu: This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar
} }
::-webkit-scrollbar-thumb { ::-webkit-scrollbar-thumb {
background-color: var(--shades-black-60); background-color: var(--shades-black-50);
border: 3px var(--shades-black-80) solid; border: 3px var(--shades-black-75) solid;
border-radius: 10px; border-radius: 10px;
} }