Page MenuHomePhabricator

[services][backup] Implement blob Put client
ClosedPublic

Authored by bartek on Jan 4 2023, 9:11 AM.
Tags
None
Referenced Files
F3287239: D6167.diff
Sat, Nov 16, 1:48 PM
Unknown Object (File)
Sun, Nov 10, 10:05 PM
Unknown Object (File)
Sat, Nov 9, 2:58 PM
Unknown Object (File)
Sat, Nov 9, 2:19 PM
Unknown Object (File)
Sat, Nov 9, 11:56 AM
Unknown Object (File)
Sat, Nov 9, 11:55 AM
Unknown Object (File)
Sat, Nov 9, 11:12 AM
Unknown Object (File)
Mon, Nov 4, 8:38 PM
Subscribers

Details

Summary

This diff implements the blob Put client logic. Added extensive in-code comments describing its working principle.

This code is an improved and simplified version of old blob_client: (link) - I got rid of custom Tokio runtimes and mutexes, which caused problems in the previous implementation.

Depends on D6166

Test Plan

This code is tested in subsequent diffs where it is used in backup service ednpoints.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Jan 4 2023, 9:24 AM
tomek added inline comments.
services/backup/src/blob/put_client.rs
92 ↗(On Diff #20607)

Can we also import anyhow?

varun requested changes to this revision.Jan 9 2023, 7:27 PM
varun added inline comments.
services/backup/src/blob/put_client.rs
34–38 ↗(On Diff #20696)

Why do we create the BlobServiceClient in PutClient and GetClient? Can we just do it once and then clone to reuse the same underlying Channel?

This revision now requires changes to proceed.Jan 9 2023, 7:27 PM
services/backup/src/blob/put_client.rs
34–38 ↗(On Diff #20696)

This sounds interesting but also looks like a significant change.
My original idea was to use one client instance per request, but your proposal seems to be reasonable.
After brief research, the above statement can be split into these:

let endpoint = tonic::transport::Channel::from_static(service_url);
let channel = endpoint.connect().await?;
let mut blob_client = BlobServiceClient::<Channel>::new(channel);

As I understand, the Channel is "encouraged to be cloned" and the BlobServiceClient is a wrapper over tonic::client::Grpc<Channel> which derives Clone so it is also cloneable.

So my questions are:

  1. Where and how would you store the client instance? Should it be provided in the same way as e.g. DatabaseClient is provided to the service? (D6178)
  2. When should it be initialized? Lazily when first used?
  3. How does Tonic handle network connection? Is it kept alive? How are disconnections handled then? Should I care about it at all? (I think I need to read more about http2)

If this is going to be a significant code structure change, I'd prefer to create a Linear task and do this as a follow-up after this stack is landed.

varun added inline comments.
services/backup/src/blob/put_client.rs
34–38 ↗(On Diff #20696)
  1. I think it makes sense to do what you did with the DatabaseClient
  2. I don't have a strong preference here... we can just initialize it main like the db client I think
  3. Under the hood there's a Hyper connection pool. Hyper will handle reestablishing connection if it's lost -- I don't think you have to worry about it.

I'm fine with addressing this in a follow-up.

This revision is now accepted and ready to land.Jan 10 2023, 7:54 AM
services/backup/src/blob/put_client.rs
34–38 ↗(On Diff #20696)

Import anyhow macro directly