Depends on D5089
Linear task: https://linear.app/comm/issue/ENG-1734/improve-error-handling-system-in-the-blob-client
Using a new way of error handling in the terminate method of the get client.
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
It would be really helpful if you provided a context behind introducing _new functions.
| services/backup/blob_client/src/get_client.rs | ||
|---|---|---|
| 38–52 ↗ | (On Diff #16572) | Why do we need two functions? Why the new one uses &String? |
| 59 ↗ | (On Diff #16572) | We should avoid committing commented out code. What is the reason for that? |
| 159 ↗ | (On Diff #16572) | According to the doc https://docs.rs/anyhow/latest/anyhow/
So we can use anyhow::Result<()> |
| 182–184 ↗ | (On Diff #16572) | We should use a macro provided by the library https://docs.rs/anyhow/latest/anyhow/macro.ensure.html |
Comment Actions
Yes, sorry, I missed doing that. I explained why I added them in https://phab.comm.dev/D5089#150612:
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.
| services/backup/blob_client/src/get_client.rs | ||
|---|---|---|
| 38–52 ↗ | (On Diff #16572) | Explained in the big comment. |
| 59 ↗ | (On Diff #16572) | will remove |
| 159 ↗ | (On Diff #16572) | right |
| 182–184 ↗ | (On Diff #16572) | bail is provided by the library too https://docs.rs/anyhow/latest/anyhow/macro.bail.html |
| services/backup/blob_client/src/get_client.rs | ||
|---|---|---|
| 182–184 ↗ | (On Diff #16572) | That's correct, but if we want to bail based on a condition, we can use another library function ensure |
Comment Actions
What do you mean by just saying "better" with no previous context?
It's an improvement to what was there before, and the style of "address comments in future revisions" means that I don't know if something that I think can be improved in this revision has been addressed already somewhere else.
My threshold for this stack is "is it moving roughly in the right direction".
Comment Actions
remove commented code
| services/backup/blob_client/src/get_client.rs | ||
|---|---|---|
| 159 ↗ | (On Diff #16572) | |
| 182–184 ↗ | (On Diff #16572) | Right, yes, this looks handy, although I don't think this is super hi-prio stuff. https://linear.app/comm/issue/ENG-1840/use-ensure-instead-of-bail |