Depends on D5095
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 initialize method of the put 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
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 48 ↗ | (On Diff #16548) | functions or values named is_XXX I feel should always return a bool. Returning a result seems very odd |
| 48–54 ↗ | (On Diff #16548) | Feel like the logic should be: |
| 106 ↗ | (On Diff #16548) | really don't like the use of panic, as this will probably cause issues going across the C++ interface. But I think this gets handled in another diff. But ideally, we wouldn't have an intermediate "worst case" |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 48 ↗ | (On Diff #16548) | It essentially returns bool though. The "error" part of the result stands for... well, error. I don't really understand what is the problem here. The "result" syntax is just an equivalent of throw in c++ let's say. |
| 48–54 ↗ | (On Diff #16548) | I don't think this is a good idea, sorry. The problem with what you proposed is that it's going to return false when we fail to lock. Failed locking should be considered an exceptional situation and handled differently than not having a client initialized. Now, with your proposal, if this function returned false, we'd not know whether it failed to access the clients or if there's no such client in the collection. |
| 106 ↗ | (On Diff #16548) | Yes, you're talking about D5097. We can do this earlier (I mean here). |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 48 ↗ | (On Diff #16688) | |
| 49–54 ↗ | (On Diff #16688) | This is a strange pattern - it would be more readable to see this match outside. Something like the suggested code, but the case for _ may be slightly different. |
| 64–66 ↗ | (On Diff #16688) | ensure |
| 115 ↗ | (On Diff #16688) | Do we have a task / plan how we will improve this? |
| 128 ↗ | (On Diff #16688) | Is there a better way to handle Result than using match? Maybe anyhow can be used for that? |
| 162 ↗ | (On Diff #16688) | ensure |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 115 ↗ | (On Diff #16688) | Please create task before landing and link here |