Depends on D3585
Add logic for storing the log data in the database.
Details
Details
cd services yarn run-backup-service
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h | ||
|---|---|---|
| 133 ↗ | (On Diff #11032) | error |
Comment Actions
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
Comment Actions
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
Comment Actions
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 |
| 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. |
| 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. |