Page MenuHomePhabricator

[services] Backup - Send log - add store in database
ClosedPublic

Authored by karol on Mar 31 2022, 6:32 AM.
Tags
None
Referenced Files
F3536218: D3586.id11391.diff
Wed, Dec 25, 5:48 PM
F3536085: D3586.id11392.diff
Wed, Dec 25, 5:28 PM
F3536080: D3586.id11088.diff
Wed, Dec 25, 5:27 PM
F3536065: D3586.id11039.diff
Wed, Dec 25, 5:22 PM
F3536061: D3586.id10999.diff
Wed, Dec 25, 5:22 PM
F3531173: D3586.diff
Wed, Dec 25, 6:36 AM
Unknown Object (File)
Sun, Dec 22, 12:35 PM
Unknown Object (File)
Fri, Dec 20, 3:34 PM

Details

Summary

Depends on D3585

Add logic for storing the log data in the database.

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

fix calling scheduleSendingDataChunk

karol added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
133 ↗(On Diff #11032)

error

tomek requested changes to this revision.Apr 7 2022, 12:02 AM

Please make the summary more descriptive. Add the rest of SendLog functionality is a really general description of what is happening here. And a lot is happening here - is there a way to split this diff? (It's also confusing what do you mean by the rest - it doesn't sound like adding new todos should be a part of a diff which adds the rest).

Add logic to make SendLog functionality fully work.

A diff like that should definitely have a concrete test plan - it looks like at this point the logic should be easy to test

This revision now requires changes to proceed.Apr 7 2022, 12:02 AM

Yes, right, I'm going to split this diff maybe

karol retitled this revision from [services] Backup - Add the rest of SendLog functionality to [services] Backup - Send log - add store in database.Apr 7 2022, 7:20 AM
karol edited the summary of this revision. (Show Details)

Ok, I split the diff into multiple diffs. I hope it's now easier for you to review, I was trying to add a simple piece of code in every diff. The test plan you'd probably want to see is in D3651

tomek requested changes to this revision.Apr 8 2022, 1:06 AM
In D3586#100172, @karol-bisztyga wrote:

Ok, I split the diff into multiple diffs. I hope it's now easier for you to review, I was trying to add a simple piece of code in every diff. The test plan you'd probably want to see is in D3651

Thanks! It's a lot better!

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

In this diff it isn't obvious why do we need to store value in this instead of receiving it via function argument

76 ↗(On Diff #11178)

This diff shouldn't touch this function. I think we can keep this call - even if the function is empty

This revision now requires changes to proceed.Apr 8 2022, 1:06 AM
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
58 ↗(On Diff #11178)

Maybe it isn't but it becomes clear in the following diffs. Could we save some time I'd spent rebasing it and let this one through?

76 ↗(On Diff #11178)

ok

tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
112 ↗(On Diff #11219)

Let's revert also this change

58 ↗(On Diff #11178)

Ok, that makes sense. Generally, there's nothing wrong in adding something and not using it, but it can cause some confusion. The solution is to catch such issues before putting up a diff and explaining in the summary or inline comments why something is added.

This revision is now accepted and ready to land.Apr 8 2022, 3:23 AM
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
55–69 ↗(On Diff #11219)

Still don't understand why we are putting implementations in .h files like this. I know @karol-bisztyga created a task to track this, but I don't understand why we ever landed code with this anti-pattern. I would like us to prioritize removing all of the implementations from .h files wherever possible

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
55–69 ↗(On Diff #11219)

I don't see a point in doing it now (in this diff) if there's a task for this. I'm going to do this task soon, so please do not worry.
The reason it is done temporarily like that is to save time in the early development stage.

This revision was landed with ongoing or failed builds.Apr 13 2022, 4:13 AM
This revision was automatically updated to reflect the committed changes.