Page MenuHomePhabricator

[services] Rust Integration - Backup - Rust - Multiple get clients
ClosedPublic

Authored by karol on Sep 2 2022, 3:52 AM.
Tags
None
Referenced Files
F3383053: D5037.id16236.diff
Thu, Nov 28, 1:14 PM
F3382838: D5037.diff
Thu, Nov 28, 12:13 PM
Unknown Object (File)
Sun, Nov 24, 10:29 PM
Unknown Object (File)
Sun, Nov 24, 9:41 PM
Unknown Object (File)
Sun, Nov 24, 9:38 PM
Unknown Object (File)
Sun, Nov 24, 9:27 PM
Unknown Object (File)
Sun, Nov 24, 9:12 PM
Unknown Object (File)
Tue, Nov 19, 1:59 PM

Details

Summary

Depends on D5036

In order to be able to handle multiple connections at the same time, we have to store a collection of clients in the rust state instead of having just one.

Here, I'm adding the collection to the get blob client.

Test Plan

The backup service will not build successfully because of invalid calls from the c++ but I decided to divide the code so the diffs are not too big. This is fixed in the next diff in the stack - D5038

This will work:

cd services/backup/blob_client
cargo check

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 2 2022, 3:56 AM
Harbormaster failed remote builds in B11831: Diff 16236!
karol edited the summary of this revision. (Show Details)
karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, max, varun.

Backup doesn't build but this is explained in the test plan section.

tomek requested changes to this revision.Sep 5 2022, 8:09 AM
tomek added inline comments.
services/backup/blob_client/src/get_client.rs
27 ↗(On Diff #16236)

Can RwLock improve the performance?

38 ↗(On Diff #16236)

This should be fixed in a diff that introduced it

146–157 ↗(On Diff #16236)

Are we holding a lock to the whole CLIENTS while we're executing blocking function?

This revision now requires changes to proceed.Sep 5 2022, 8:09 AM
services/backup/blob_client/src/get_client.rs
27 ↗(On Diff #16236)
38 ↗(On Diff #16236)

I think this change should be reverted actually.

146–157 ↗(On Diff #16236)

Apparently. We may want to re-check the performance possible issues like this.

karol edited the summary of this revision. (Show Details)

revert incorrect change

tomek added inline comments.
services/backup/blob_client/src/get_client.rs
146–157 ↗(On Diff #16236)

Ok, so let's do that. If we're locking here that might affect performance significantly.

This revision is now accepted and ready to land.Sep 9 2022, 5:11 AM
services/backup/blob_client/src/get_client.rs
146–157 ↗(On Diff #16236)

Do we have a task for this investigation?

The code looks ok, but I'm confused why this diff introduces things that are replaced later. Especially, raw pointers and unsafe functions. Is this diff going to be updated?

Also the CI fails for this diff.

services/backup/blob_client/src/get_client.rs
36 ↗(On Diff #16686)

We're not mutating, so &str is more appropriate

Also the CI fails for this diff.

The CI fails because we don't use the changes from rust in c++ yet, we do that in D5038 but I didn't want to make this diff too big. On the other hand, the diff philosophy is that every single diff should not break the master branch... To speed things up, I'll just land them together, sorry, maybe I made a mistake and should've done that differently.

services/backup/blob_client/src/get_client.rs
36 ↗(On Diff #16686)
This revision was landed with ongoing or failed builds.Sep 16 2022, 1:55 AM
This revision was automatically updated to reflect the committed changes.