Depends on D5010
Adding write function to the blob put client.
Details
Details
backup builds
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 | ||
|---|---|---|
| 228 ↗ | (On Diff #16171) | I really don't like this usage of a global variable to check some implicit global mutable state. Why do we need to check for errors here? and is here really where we want to address errors inside of a put call? |
| 250 ↗ | (On Diff #16171) | report_error should really take a &str, so we don't have to do heap allocation from the callee. |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 228 ↗ | (On Diff #16171) | I realize you're not a fan of the solution I created for handling errors in the async functions but one place to discuss this in the stack should be enough https://phab.comm.dev/D5005#146417 |
| 250 ↗ | (On Diff #16171) | Yes, definitely, it is fixed later in the stack D5030 |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 234–248 ↗ | (On Diff #16171) | Is there a way to avoid this pattern? if let Some(client) = (*maybe_client).take() {
...
*maybe_client = Some(client);
} |
| 250 ↗ | (On Diff #16171) | When a diff that introduced something is still open, we should prefer fixing there instead of creating a followup diff. A reasonable reason for not following this rule is really expensive rebase, but I don't expect something like that in this case. |
| 256 ↗ | (On Diff #16171) | Returning Ok when we had an error doesn't sound perfect. |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 234–248 ↗ | (On Diff #16171) | Ok, now I see this is going to be questioned in every diff. Ok then, I'm going to create a task for this: https://linear.app/comm/issue/ENG-1736/try-to-avoid-reassigning-when-modifying-the-field-of-a-global-struct |
| 256 ↗ | (On Diff #16171) | Gonna change, thanks. |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 234–248 ↗ | (On Diff #16171) | Yeah, I've mentioned this in a couple of places to make it easier to remember about addressing this in all relevant places. We can discuss it wherever you prefer. |
Comment Actions
I agree with @tomek, I would really like to see the diff not introduce things that need to be addressed, and then have a diff later address it.
We should probably have done more "foundation" work on logging and other canonical practices before creating such a large stack of "needing to be addressed concerns"
Comment Actions
We should probably have done more "foundation" work on logging and other canonical practices before creating such a large stack of "needing to be addressed concerns"
I asked about such a thing in the rust comm channel some time ago and didn't really get any precise answers. This is why I also thought it'd be better for one person to really work out the rust stuff first, then such things as logging would be available for others from the start.