Page MenuHomePhabricator

[services] Backup - Create new backup - Fix checking final status logic
ClosedPublic

Authored by karol on May 31 2022, 6:48 AM.
Tags
None
Referenced Files
F3356915: D4160.id13447.diff
Sat, Nov 23, 9:27 PM
Unknown Object (File)
Thu, Nov 14, 4:40 PM
Unknown Object (File)
Thu, Nov 14, 4:40 PM
Unknown Object (File)
Thu, Nov 14, 4:40 PM
Unknown Object (File)
Thu, Nov 14, 4:40 PM
Unknown Object (File)
Thu, Nov 14, 4:40 PM
Unknown Object (File)
Mon, Nov 11, 11:53 PM
Unknown Object (File)
Mon, Nov 11, 10:20 PM

Details

Summary

This simplifies the logic for checking the final status when creating a new backup in the backup service.

Test Plan

You can test this using my repo grpc-playground and intentionally launching only the backup service (without a blob service in another shell). Then, if you try to send a new backup, you'll see it works properly.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added a reviewer: tomek.
tomek requested changes to this revision.Jun 1 2022, 6:16 AM
tomek added inline comments.
services/backup/src/Reactors/server/CreateNewBackupReactor.cpp
91 ↗(On Diff #13232)

Why it was necessary to have state == ReactorState::DONE? Why it is no longer necessary?

This revision now requires changes to proceed.Jun 1 2022, 6:16 AM
services/backup/src/Reactors/server/CreateNewBackupReactor.cpp
91 ↗(On Diff #13232)

It's unnecessary because we know it's done - we wait until the done callback is called. But ok, we can add a sanity check here.

services/backup/src/Reactors/server/CreateNewBackupReactor.cpp
88–91 ↗(On Diff #13348)

Maybe it looks odd to have two distinct consecutive ifs with the same condition but it makes sense in this case.

tomek added inline comments.
services/backup/src/Reactors/server/CreateNewBackupReactor.cpp
88–93 ↗(On Diff #13348)

This pattern is really risky, but in our case it's probably fine. The main risk is due to spurious wakeup - if there are a couple of threads waiting on the same condition, it is possible that more than one will be waken up. In our case only one thread waits, so it's fine.

This revision is now accepted and ready to land.Jun 7 2022, 1:41 AM
services/backup/src/Reactors/server/CreateNewBackupReactor.cpp
88–93 ↗(On Diff #13348)

yeah, it's a 1v1 relation basically