Details

Reviewers

tomek
varun
ashoat

Commits

rCOMM0348a8fba605: [services] Tests - Update backup test

Summary

Depends on D4324

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

I added a case when we first add an item, it is stored in the database, then we add so many attachment holders, that it goes beyond the dynamo limit and we move this data to the S3.

Test Plan

cd services
yarn run-integration-tests backup

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

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

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

Harbormaster completed remote builds in B9875: Diff 13660.Jun 22 2022, 4:05 AM

• karol requested review of this revision.Jun 22 2022, 4:05 AM

• karol mentioned this in D4233: [services] Backup - Fix data size database limit.Jun 22 2022, 5:37 AM

• karol edited the summary of this revision. (Show Details)Jun 22 2022, 6:33 AM

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

• karol added reviewers: tomek, varun, ashoat.

• karol added a parent revision: D4325: [services] Tests - Backup - Add dynamoDB item size limit.

I don't feel confident reviewing Rust :(

• karol added a child revision: D4341: [services] Backup - Pull Backup - Wait for the get reactor to terminate.Jun 23 2022, 4:16 AM

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

tomek added inline comments.

services/commtest/tests/backup_test.rs
17 ↗	(On Diff #13660)	According to https://doc.rust-lang.org/book/ch07-04-bringing-paths-into-scope-with-the-use-keyword.html#creating-idiomatic-use-paths it is a good practice to not import a function directly - we should instead import its parent module. Bringing the function’s parent module into scope with use means we have to specify the parent module when calling the function. Specifying the parent module when calling the function makes it clear that the function isn’t locally defined while still minimizing repetition of the full path. On the other hand, when bringing in structs, enums, and other items with use, it’s idiomatic to specify the full path.
134 ↗	(On Diff #13660)	Not sure what's better, but when creating an empty vector we can simply use a constructor
135 ↗	(On Diff #13660)	It would be a good idea to connect two magic numbers `500` and `300`, so that changing one would cause the other one to be changed. We can e.g. create a variable for `300` and use that variable + 200 here.
151 ↗	(On Diff #13660)	What do you think about using `enumerate` here?
154 ↗	(On Diff #13660)	`assert_eq`?

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

• karol added inline comments.Jun 30 2022, 7:07 AM

services/commtest/tests/backup_test.rs
17 ↗	(On Diff #13660)	ok, I'm good with this.
134 ↗	(On Diff #13660)	I can change this, I don't think that matters.
151 ↗	(On Diff #13660)	I think that would be confusing. We're using data from two collections at the same time. We'd need to access one by index and the other one by name (name used in the for loop taken from the enumeration).
154 ↗	(On Diff #13660)	right

address feedback

tomek accepted this revision.Jun 30 2022, 7:40 AM

tomek added inline comments.

services/commtest/tests/backup_test.rs
155–162 ↗	(On Diff #14034)	Not sure if specifying `expected` and `from_result` twice makes sense
167–190 ↗	(On Diff #14034)	This code is copied from 30 lines above it - could you find a way to avoid this duplication?