Page MenuHomePhabricator

[services] Backup - Blob Put Client - Add initialize
ClosedPublic

Authored by karol on Sep 1 2022, 4:07 AM.
Tags
None
Referenced Files
F3364747: D5009.diff
Mon, Nov 25, 5:04 AM
Unknown Object (File)
Fri, Nov 22, 7:31 AM
Unknown Object (File)
Sat, Nov 16, 4:32 PM
Unknown Object (File)
Wed, Nov 13, 1:10 PM
Unknown Object (File)
Sun, Nov 10, 6:57 AM
Unknown Object (File)
Mon, Nov 4, 6:20 PM
Unknown Object (File)
Sun, Oct 27, 8:11 PM
Unknown Object (File)
Sun, Oct 27, 8:11 PM

Details

Summary

Depends on D5008

Adding initialize function to the blob put client.

Test Plan

backup builds

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Sep 5 2022, 5:07 AM
tomek added inline comments.
services/backup/blob_client/src/put_client.rs
82–85 ↗(On Diff #16243)

Is it possible for sender to have a different type than receiver? It would be strange... so maybe there's a way to have a type parameter provided only once?

91–92 ↗(On Diff #16243)

Can we directly use Result?

146–171 ↗(On Diff #16243)

This code can be simplified significantly by replacing response_present variable with a simple break statement

173–176 ↗(On Diff #16243)

Not really sure whether introducing maybe_response is a good idea. Creating it causes an error reported twice: once when we create it and match Err and then here.

199–201 ↗(On Diff #16243)

Can we update this in a diff that introduced this comment?

This revision now requires changes to proceed.Sep 5 2022, 5:07 AM
services/backup/blob_client/src/put_client.rs
82–85 ↗(On Diff #16243)

I don't think that's possible (that they have different types), and I'm also not sure if one could really specify the types only once. I've never seen anything like it, maybe @varun or @jon know something about this?

91–92 ↗(On Diff #16243)

right

146–171 ↗(On Diff #16243)

I agree it can, although I wouldn't call it a significant simplification, it makes this code thinner by just some words. For me, the current version is clearer but I don't mind changing it.

173–176 ↗(On Diff #16243)

Ehh, yes, that's right. This one time I wanted to make the rust code less indented, but ok, will change.

199–201 ↗(On Diff #16243)

yes

tomek added inline comments.
services/backup/blob_client/src/put_client.rs
82–85 ↗(On Diff #16243)

Have you tried something like this?

let (request_thread_tx, mut request_thread_rx) = mpsc::channel::<PutRequestData>(MPSC_CHANNEL_BUFFER_CAPACITY);
91–92 ↗(On Diff #16243)

Can we do this in other places too?

199–201 ↗(On Diff #16243)

I can see that D5004 contains updated values, but this diff still shows this as changed, so the diff should be probably rebased.

This revision is now accepted and ready to land.Sep 6 2022, 6:51 AM
services/backup/blob_client/src/put_client.rs
82–85 ↗(On Diff #16243)

I'll try this, thanks.

91–92 ↗(On Diff #16243)

yes, I'll do it eventually, thanks!

services/backup/blob_client/src/put_client.rs
82–85 ↗(On Diff #16243)

This worked

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.