Page MenuHomePhabricator

[services] Backup - Send log - add terminate callback
ClosedPublic

Authored by karol on Apr 7 2022, 7:22 AM.
Tags
None
Referenced Files
F3377448: D3650.diff
Wed, Nov 27, 5:44 AM
Unknown Object (File)
Fri, Nov 22, 12:43 AM
Unknown Object (File)
Wed, Nov 20, 3:33 PM
Unknown Object (File)
Fri, Nov 15, 2:07 AM
Unknown Object (File)
Wed, Nov 13, 11:28 AM
Unknown Object (File)
Thu, Nov 7, 9:01 AM
Unknown Object (File)
Tue, Nov 5, 5:47 PM
Unknown Object (File)
Fri, Nov 1, 7:45 AM

Details

Summary

Depends on D3649

Adding a terminate callback so we wait until the chunks upload is finished (if it ever should be performed in a current case).

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Apr 8 2022, 1:51 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
165–169 ↗(On Diff #11174)

I know we're going to search for a better alternative, but wanted to emphasize that this solution is fragile. We don't have any guarantees about when the scheduled write will finish.

Is this possible to have a callback added to scheduleSendingDataChunk which will be called when the operation is finished? We could call storeInDatabase in this callback.

This revision now requires changes to proceed.Apr 8 2022, 1:51 AM
karol added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
165–169 ↗(On Diff #11174)

Hmm, could you please throw some light on what exactly will be different? Here we're waiting with the CV, you're suggesting a callback. These are two solutions to the same problem and I don't really see a difference in the outcome. In other words, how would using a callback make a difference for this:

this solution is fragile. We don't have any guarantees about when the scheduled write will finish.

?
I'm probably missing something, but it would be cool if you could point out what is it. Thanks.

karol edited the summary of this revision. (Show Details)

rebase

tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
165–169 ↗(On Diff #11174)

Overall, writing the code that uses synchronization is a lot more complicated - there are multiple unintuitive mistakes that can easily be made without deep understanding. On the other hand, using a callback is intuitive.

It also helps with readability - we're explicit about dependencies.

But overall, if this callback need a synchronization, then most of its advantages are not valid.

This revision is now accepted and ready to land.Apr 12 2022, 9:21 AM
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
165–169 ↗(On Diff #11174)

I think the callback would need to be synced anyways - We cannot just call it whenever we like, at some point we have to wait until the operations on the other reactor are finished (so in the case you're describing, we'd have to wait in the other reactor, not here).