Details

Reviewers

Commits

Summary

Depends on D5092

Using a new way of error handling in the terminate method of the put client.

Test Plan

cd services/backup/blob_client
cargo check

after running localstack, blob and backup (in this particular order), this still works:

yarn run-performance-tests backup

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

• karol requested review of this revision.Sep 9 2022, 4:54 AM

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

• jon requested changes to this revision.Sep 9 2022, 10:52 AM

• jon added inline comments.

services/backup/blob_client/src/put_client.rs
59 ↗	(On Diff #16545)	what's the purpose of holder? also `&String` should always be `&str` in function inputs, as `String` and `&String` should be able to AsRef into a `&str`.

This revision now requires changes to proceed.Sep 9 2022, 10:52 AM

services/backup/blob_client/src/put_client.rs
59 ↗	(On Diff #16545)	By `holder` we're identifying the client we're referring to. Sure, we can use `&str`.

update

This revision is now accepted and ready to land.Sep 12 2022, 9:05 AM

tomek requested changes to this revision.Sep 15 2022, 8:46 AM

tomek added inline comments.

services/backup/blob_client/src/put_client.rs
36 ↗	(On Diff #16574)	What does this comment mean?
59 ↗	(On Diff #16574)	D5090 introduces `is_initialized_new(holder: &String)` function, later in the stack in D5092 this function is deleted and `is_initialized(holder: &str)` is used instead, but then this diff introduces `is_initialized_new` again but with a different parameter type. Something is wrong here.

This revision now requires changes to proceed.Sep 15 2022, 8:46 AM

services/backup/blob_client/src/put_client.rs
36 ↗	(On Diff #16574)	should be removed
59 ↗	(On Diff #16574)	You missed that the 2 diffs you mentioned do changes in get client and this diff does changes in put client. So I don't put something back again really.