Details

Reviewers

• jon
ashoat
tomek
varun

Commits

rCOMM005bd9ece94e: [services] Backup - Blob Get Client - Prepare for error handling

Summary

Depends on D5074

Linear task: https://linear.app/comm/issue/ENG-1734/improve-error-handling-system-in-the-blob-client

Preparing for the upcoming changes in error handling. I'm going to be using anyhow, I saw we use it in the tunnelbroker as well.

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

Event Timeline

• karol created this revision.Sep 9 2022, 4:42 AM

Herald added a reviewer: • jon. · View Herald TranscriptSep 9 2022, 4:42 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• karol edited the summary of this revision. (Show Details)Sep 9 2022, 4:50 AM

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

• karol added reviewers: ashoat, tomek, varun.

• karol added a parent revision: D5074: [services] Rust Integration - Backup - c++ - Fix wrong order of attachment holders in tests.

• karol added inline comments.

services/backup/blob_client/Cargo.toml
16 ↗	(On Diff #16541)

Harbormaster completed remote builds in B12042: Diff 16541.Sep 9 2022, 4:51 AM

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

• karol planned changes to this revision.Sep 9 2022, 4:54 AM

• karol added a child revision: D5090: [services] Backup - Blob Get Client - Refactor get terminate.

fix version

Harbormaster completed remote builds in B12052: Diff 16551.Sep 9 2022, 5:06 AM

feel like the linting stuff should have been applied to the respective revisions. Also feel like the "create more revisions to apply feedback" is creating more churn for everyone, instead of applying the feedback in the original revision, then doing a rebase after applying the feedback.

Also really jarring when the differential queue is out-of-order, and feels like im reviewing fixes for problems that were introduced by an earlier diff.

Feel like this is an intermediate commit, guess just deal with it.

This revision is now accepted and ready to land.Sep 14 2022, 11:04 AM

feel like the linting stuff should have been applied to the respective revisions. Also feel like the "create more revisions to apply feedback" is creating more churn for everyone, instead of applying the feedback in the original revision, then doing a rebase after applying the feedback.

Strongly agree!

services/backup/blob_client/src/lib.rs
35–40 ↗	(On Diff #16551)	Each diff should be properly formatted. Could you update the diff where this was introduced so that the formatting is correct? Maybe we should introduce a CI check for this?
services/backup/blob_client/src/tools.rs
44–65 ↗	(On Diff #16551)	Are these needed when we prefer using `&str` where possible? I think it should be possible to avoid raw pointers entirely.

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

In D5089#150511, @tomek wrote:

feel like the linting stuff should have been applied to the respective revisions. Also feel like the "create more revisions to apply feedback" is creating more churn for everyone, instead of applying the feedback in the original revision, then doing a rebase after applying the feedback.

Strongly agree!

I understand. I still think that this phabricator/diff workflow is not good at all for implementing large functionalities in an agile environment.

services/backup/blob_client/src/tools.rs
44–65 ↗	(On Diff #16551)	Are these needed when we prefer using &str where possible? This is a middle-state so the `cargo check` passes. It's a tradeoff. If I removed the old functions and added new ones right away, I'd have to refactor the code in all places and the diff would be far too big. I think it should be possible to avoid raw pointers entirely. Yes, this will be done in D5099