Details

Reviewers

tomek
ashoat

Commits

rCOMM195b897c1f35: [services] Backup - Update add attachments logic

Summary

Depends on D4322

linear task: https://linear.app/comm/issue/ENG-1288/properly-handle-the-dynamodb-item-size-limit

Every time we add attachments to the log item, we check if now they are still small enough to be kept in the database, and if they're not, we move their data to the S3.

Test Plan

Services build

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Jun 22 2022, 3:31 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJun 22 2022, 3:31 AM

Harbormaster completed remote builds in B9872: Diff 13657.Jun 22 2022, 3:50 AM

• karol requested review of this revision.Jun 22 2022, 3:50 AM

• karol planned changes to this revision.Jun 22 2022, 6:11 AM

• karol edited the summary of this revision. (Show Details)

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, ashoat.

• karol added a parent revision: D4322: [services] Backup - Add data hash to database manager.

• karol added inline comments.

services/backup/src/BackupServiceImpl.cpp
84–85	There should also be a condition checking if this is already stored in blob. If so, we don't have to check the size.

• karol added inline comments.Jun 22 2022, 6:23 AM

services/backup/src/BackupServiceImpl.cpp
94	also here there should be just `true`

hardcode true, don't check if the item is already on S3

• karol added a child revision: D4324: [services] Backup - Fix dynamoDB item size limit.Jun 22 2022, 6:27 AM

Harbormaster completed remote builds in B9879: Diff 13664.Jun 22 2022, 6:29 AM

Don't have time to do a full review right now, defer to @palys-swm on most of it

services/backup/src/BackupServiceImpl.cpp
93 ↗	(On Diff #13664)	This code is so deeply nested, it feels hard to follow. Is there any way to reduce the nesting?

tomek requested changes to this revision.Jun 23 2022, 6:35 AM

tomek added inline comments.

services/backup/src/BackupServiceImpl.cpp
85–86 ↗	(On Diff #13664)	Is it ever possible for database to return an item that exceeds the size limit?
92 ↗	(On Diff #13664)	Maybe use a new name, e.g. `blobLogItem`. I think that reusing a name is harmful for readability
93 ↗	(On Diff #13664)	I think the best option to reduce nesting and improve readability is to split this into functions

This revision now requires changes to proceed.Jun 23 2022, 6:35 AM

• karol added inline comments.Jun 30 2022, 5:01 AM

services/backup/src/BackupServiceImpl.cpp
85–86 ↗	(On Diff #13664)	No, but please note, that after `find` and before this check, we do `logItem->addAttachmentHolders(holders);` and this very case is handled here - if we add attachments and by adding them, we exceed the limit, we should move the data to the S3.
92 ↗	(On Diff #13664)	We need this. Please, note that we use `logItem` for putting it back into the database with any modifications that have been applied: database::DatabaseManager::getInstance().putLogItem(*logItem);
93 ↗	(On Diff #13664)	ok we can decompose

decompose as suggested

Splitting the logic into functions improved the readability significantly - thanks!

services/backup/src/Reactors/server/AddAttachmentsReactor.h
15 ↗	(On Diff #14029)	I think that all the `Reactor`s need to implement some interface from grpc. We don't do anything like that here, so we should find a name for this class that doesn't contain `Reactor`.

This revision is now accepted and ready to land.Jun 30 2022, 7:20 AM

Harbormaster failed remote builds in B10159: Diff 14029!Jun 30 2022, 8:42 AM

Harbormaster completed remote builds in B10159: Diff 14029.Jun 30 2022, 8:48 AM

ashoat added inline comments.Jun 30 2022, 1:02 PM

services/backup/src/Reactors/server/AddAttachmentsReactor.h
15 ↗	(On Diff #14029)	Agree with this

there is a bug in this code

services/backup/src/Reactors/server/AddAttachmentsReactor.cpp
57 ↗	(On Diff #14029)	a new pointer should be returned from this function
62 ↗	(On Diff #14029)	if we do this and we do not return, the outer item will not update