Depends on D5034
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 put blob client.
Details
Details
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 - D5036
This will work:
cd services/backup/blob_client cargo check
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Overall this might work but it seems like having a pool of clients and a queue with tasks could be a better solution.
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 41 ↗ | (On Diff #16234) | This probably could be made more efficient by using RwLock |
| 102–108 ↗ | (On Diff #16234) | We should format this code in the diff that introduced it. |
| 305 ↗ | (On Diff #16234) | Can we make this message more verbose instead of adding just numbers? |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 41 ↗ | (On Diff #16234) | I don't know, we probably should discuss that, mutex seems to be the easiest solution. I think this needs more research/discussion. https://linear.app/comm/issue/ENG-1741/consider-using-rwlock-instead-of-mutex |
| 102–108 ↗ | (On Diff #16234) | you're right |
| 305 ↗ | (On Diff #16234) | sure |
Comment Actions
I don't think an approach where every function takes the same parameter is a maintainable one. If all the functions share the same context, we should probably make this context a field of a struct and implement all the functions as methods of this struct. Is it possible to do something like that in cxx?
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 49–54 ↗ | (On Diff #16367) | Can we keep the approach with match? |
| 140–143 ↗ | (On Diff #16367) | Please avoid formatting the code that was introduced earlier |
| services/backup/blob_client/src/lib.rs | ||
|---|---|---|
| 18–20 ↗ | (On Diff #16496) | Is there a good reason for using *const c_char? Are there any other CXX types that represent a string and don't require using raw pointers? |
| services/backup/blob_client/src/put_client.rs | ||
| 47 ↗ | (On Diff #16496) | Is there a downside of using &str? |
| services/backup/blob_client/src/lib.rs | ||
|---|---|---|
| 18–20 ↗ | (On Diff #16496) | In all tutorials they use const char* but you made me think and I don't see why rust::String couldn't work. It is derived in the cxx bridge header file, so it is accessible and can be constructed from the c++ std::string. I've not tried it though. I'm going to give it a try, thanks. |
| services/backup/blob_client/src/put_client.rs | ||
| 47 ↗ | (On Diff #16496) | And is there one of using &String? I think it's better to borrow a heap-allocated string instead of converting it into a str, right? |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 47 ↗ | (On Diff #16496) | Could also use rust::Str https://cxx.rs/binding/str.html, and then this would be &str on the rust side which is more appropriate. |
Comment Actions
The CI reported some compilation issues
| services/backup/blob_client/src/lib.rs | ||
|---|---|---|
| 18–20 ↗ | (On Diff #16496) | It would be a lot better to update this diff instead of creating a followup |
| services/backup/blob_client/src/put_client.rs | ||
| 40 ↗ | (On Diff #16496) | Can we update this diff to not use Arc? |
| 47 ↗ | (On Diff #16496) |
Yet, it is
So using &str in every place where it can be used is a good practice because it makes the API easier to use. On the other hand
So if the function mutates, then String is a good idea. But in all the cases &str is strongly preferred, unless it can't be used. |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 40 ↗ | (On Diff #16496) | https://phab.comm.dev/D5114 was merged, should just need to rebase now |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 47 ↗ | (On Diff #16496) | Ok, fair enough, will change to &str then. |
Comment Actions
would really like for this to be rebased, the Arc usage for the other lazy_static items has been removed on master, and would like to avoid potentially adding them back.
Comment Actions
Btw, if that was the only concern I think you could just accept it, rebasing to master was pretty easy and straightforward and would enable landing some diffs already reducing cycles.
Comment Actions
Not sure what happened here, but according to CI
This revision was landed with ongoing or failed builds.
We should (almost) never land anything with failed builds!