Page MenuHomePhabricator

[services] Backup - Send log - Add basic logic
ClosedPublic

Authored by karol on Mar 30 2022, 5:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 29, 4:02 PM
Unknown Object (File)
Wed, May 29, 4:02 PM
Unknown Object (File)
Wed, May 29, 4:02 PM
Unknown Object (File)
Wed, May 29, 4:02 PM
Unknown Object (File)
Wed, May 29, 4:02 PM
Unknown Object (File)
Wed, May 29, 4:02 PM
Unknown Object (File)
Wed, May 29, 3:57 PM
Unknown Object (File)
Wed, May 29, 2:34 PM

Details

Summary

Depends on D3535

Add a simple algorithm for handling receiving a new log. This is going to be extended in future diffs.

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 1 2022, 4:38 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Constants.h
33–34 ↗(On Diff #10809)

This comment should explain why smaller size allows us to do so. Or it could explain why it is not possible when the size is greater.

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
77–82 ↗(On Diff #10809)

Have you tested this code?

We always throw here:
if persistenceMethod = PersistenceMethod::UNKNOWN we throw because persistenceMethod != PersistenceMethod::BLOB
if persistenceMethod = PersistenceMethod::BLOB we throw because persistenceMethod != PersistenceMethod::UNKNOWN
in all the other cases we throw because persistenceMethod != PersistenceMethod::UNKNOWN

This revision now requires changes to proceed.Apr 1 2022, 4:38 AM
services/backup/docker-server/contents/server/src/Constants.h
33–34 ↗(On Diff #10809)

ok

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
77–82 ↗(On Diff #10809)

I saw this issue before, there should be &&, not ||. I thought I updated this but apparently, I did not, thanks for spotting this!

update comment, fix condition

tomek requested changes to this revision.Apr 4 2022, 4:21 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Constants.h
33–37 ↗(On Diff #10986)

Please use sentences (capital letters and dots).

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
68–88 ↗(On Diff #10986)

I'm not sure if I understand the idea behind this, but this doesn't seem to be correct. Let's suppose we want to persist two chunks, one bigger than LOG_DATA_SIZE_DATABASE_LIMIT and one smaller. After the first chunk, we set this->persistenceMethod = PersistenceMethod::BLOB. When processing the second, the first if condition is true, and we throw because this->persistenceMethod != PersistenceMethod::UNKNOWN. Am I missing something?

As a side note, it's better to avoid throwing when we can recover from invalid state. In this case, if we started writing to the db and then the size limit is exceeded, we can move the data to blob and write the rest there.

This revision now requires changes to proceed.Apr 4 2022, 4:21 AM

update logic

services/backup/docker-server/contents/server/src/Constants.h
33–37 ↗(On Diff #10986)

ok

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
68–88 ↗(On Diff #10986)

You are right, there's a logic error here, I'm going to fix this.

As a side note, it's better to avoid throwing when we can recover from invalid state. In this case, if we started writing to the db and then the size limit is exceeded, we can move the data to blob and write the rest there.

This would make sense but I think not in this particular case. The idea here is that storing in DB is for one-chunk operations only. If we ever receive more than one chunk, we should never store the bytes in the DB.

tomek requested changes to this revision.Apr 5 2022, 9:35 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
72–80 ↗(On Diff #11031)

Do we need e.g. else here? I guess the intention wasn't to always throw.

This revision now requires changes to proceed.Apr 5 2022, 9:35 AM
services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
72–80 ↗(On Diff #11031)

Ahh, right!

This revision is now accepted and ready to land.Apr 6 2022, 6:52 AM