• 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 ↗	(On Diff #10546)	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.

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

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.

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