Page MenuHomePhabricator

[services] Rust Integration - Backup - Make report_error shared
ClosedPublic

Authored by karol on Sep 1 2022, 11:54 PM.
Tags
None
Referenced Files
F2102019: D5030.id16226.diff
Mon, Jun 24, 11:25 PM
Unknown Object (File)
Sun, Jun 23, 4:29 AM
Unknown Object (File)
Sat, Jun 22, 10:32 AM
Unknown Object (File)
Fri, Jun 21, 8:13 PM
Unknown Object (File)
Fri, Jun 21, 7:01 PM
Unknown Object (File)
Wed, Jun 19, 12:23 AM
Unknown Object (File)
Tue, Jun 18, 11:58 AM
Unknown Object (File)
Sat, Jun 15, 10:45 AM

Details

Summary

Depends on D5029

I just moved the function report_error to the mutual space tools following the DRY rule.

Test Plan
cd services
yarn run-integration-tests backup

Works as before

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, max, varun.
karol edited the summary of this revision. (Show Details)
jon requested changes to this revision.Sep 2 2022, 11:11 AM
jon added inline comments.
services/backup/blob_client/src/tools.rs
5 ↗(On Diff #16239)

Why pass a global variable? It's always going to be available.

Taking a step back, we shouldn't be using global variables; if we just want to see if something is in a valid state, we should attach the state to the respective resource (e.g. client).

9–12 ↗(On Diff #16239)

what's the value that labels provide? Couldn't the message just contain it.

If we were using log statements, one could also pass a target argument.

We would just need to initiate the logger alongside the runtime.

https://docs.rs/log/latest/log/macro.info.html

This revision now requires changes to proceed.Sep 2 2022, 11:11 AM
karol added inline comments.
services/backup/blob_client/src/tools.rs
5 ↗(On Diff #16239)
9–12 ↗(On Diff #16239)

I think we can do this in this task https://linear.app/comm/issue/ENG-1717/use-tracing-utilities-instead-of-println

I'm going to add a note.

services/backup/blob_client/src/tools.rs
5 ↗(On Diff #16239)

I think this is kind of side stepping the issue.

I think the current complexity could be reduced by just emitting the errors and representing the check_errors() as checking to see if a Mutex<bool> is true.

Even better would be marking the individual clients as healthy and have some logic to get them back to healthy, but that's more work, and should be done later as another task.

This revision is now accepted and ready to land.Sep 7 2022, 11:18 AM
This revision was landed with ongoing or failed builds.Sep 8 2022, 4:10 AM
This revision was automatically updated to reflect the committed changes.