Details

• karol retitled this revision from [services] Blob - Add server reactor implementations - put reactor to [draft] [services] Blob - Add server reactor implementations - put reactor.Mar 24 2022, 2:47 AM

• karol mentioned this in D3464: [services] Backup - Add server reactor base classes - bidi reactor.Mar 24 2022, 7:44 AM

ashoat requested changes to this revision.Mar 24 2022, 1:35 PM

ashoat added inline comments.

services/blob/docker-server/contents/server/src/Reactors/server/PutReactor.h
29 ↗	(On Diff #10648)	It looks like first we send a message to set the `holder`, and then we send a message to set the `blobHash`. Can we set these at the same time, and avoid the extra round trips?
81–97 ↗	(On Diff #10648)	Indentation can be reduced here

This revision now requires changes to proceed.Mar 24 2022, 1:35 PM

rebase to master

thx for the comments, I'm gonna address them later, for now, I needed that rebase.

services/blob/docker-server/contents/server/src/Reactors/server/PutReactor.h
29 ↗	(On Diff #10648)	We can indeed.
81–97 ↗	(On Diff #10648)	right, thx

• karol planned changes to this revision.Mar 25 2022, 7:52 AM

Harbormaster completed remote builds in B7614: Diff 10690.Mar 25 2022, 8:51 AM

It looks like this code was moved from D3471 - it would be really helpful if you could mention cases like that, so that the review process is simplified.

In D3521#96040, @palys-swm wrote:

It looks like this code was moved from D3471 - it would be really helpful if you could mention cases like that, so that the review process is simplified.

You're partially right, but please note that the flow changes, instead of the loop we base here on the callback, so I think it is good to check once again if everything is implemented properly. But you're right that I could've said this is based on D3471 so we know the business logic should be the same.

• karol added inline comments.Mar 28 2022, 1:55 AM

services/blob/docker-server/contents/server/src/Reactors/server/PutReactor.h
29 ↗	(On Diff #10648)	Hey, getting back to this. I don't think this is a good place to change that. In this diff, we want to refactor from sync to async API and what you're mentioning is changing the structure/API of the service. Such doubt should have been raised in D2666 and taken care of there. If you feel like changing this as you mentioned, I can create a task for this and do it separately. Please, let me know if it is important to you and should I create a task.

update

Harbormaster completed remote builds in B7632: Diff 10720.Mar 28 2022, 4:09 AM

• karol planned changes to this revision.Mar 28 2022, 5:52 AM

fix build

Harbormaster completed remote builds in B7643: Diff 10731.Mar 28 2022, 5:59 AM

ashoat accepted this revision.Mar 28 2022, 10:40 AM

ashoat added inline comments.

services/blob/docker-server/contents/server/src/Reactors/server/PutReactor.h
29 ↗	(On Diff #10648)	That makes sense – this will require `.proto` changes`, so it should be a separate diff. Yes, I would like to prioritize this – can please create a task? In general, we should be thoughtful about avoiding unnecessary round trips. I think we did a good job with this in the backup service architecture review, but looks like we missed one thing in the blob service architecture review.