Page MenuHomePhabricator

[services] Backup - Send log - finish read request logic
ClosedPublic

Authored by karol on Apr 7 2022, 7:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 6:35 AM
Unknown Object (File)
Sat, Nov 16, 3:07 PM
Unknown Object (File)
Tue, Nov 5, 5:47 PM
Unknown Object (File)
Tue, Oct 29, 7:25 AM
Unknown Object (File)
Oct 25 2024, 3:30 PM
Unknown Object (File)
Oct 21 2024, 2:35 AM
Unknown Object (File)
Oct 13 2024, 4:00 PM
Unknown Object (File)
Oct 13 2024, 4:00 PM

Details

Summary

Depends on D3650

Adding logic for certain state options in order to properly handle sending logs to the backup service.

Test Plan

You need 3 terminals
1:

cd services
yarn run-backup-service

2:

cd services
yarn run-blob-service

3:
You can use https://github.com/karol-bisztyga/grpc-playground/tree/backup-async

./build.sh
./cmake/build/bin/client

then in the GUI use the n option (just type n and hit enter, this stands for creating a new backup).

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 reviewers: tomek, max.
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗(On Diff #11175)

I'm wondering if it was a good idea to store either log or holder in value. Maybe the code would be more readable if we had two separate fields. What do you think?

This revision is now accepted and ready to land.Apr 8 2022, 1:56 AM
karol edited the summary of this revision. (Show Details)

rebase

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗(On Diff #11175)

Well, maybe, Basically, this was agreed on during the arch review. Maybe you're right. If you feel like it, please create a task and assign it to me, we can discuss if it is a good change.

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗(On Diff #11175)

It was just an idea. If you think it would simplify things, we can do this as a followup. If you're not convinced, then creating a task doesn't make a lot of sense. So up to you.

But overall, you've written this code, so do you think that having separate fields would simplify the implementation?

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗(On Diff #11175)

Basically, this was agreed on during the arch review

It's true we agreed on using the same database field for these two values, but that doesn't mean we need to have the same variable for it in SendLogReactor. I agree with @palys-swm that it would be more readable to use different variables for these

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗(On Diff #11175)