Page MenuHomePhabricator

[DRAFT] [services] Backup - Connect to Blob - Prepare for bidi stream
AbandonedPublic

Authored by karol on Aug 24 2022, 5:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 5:00 PM
Unknown Object (File)
Sat, Dec 14, 7:02 AM
Unknown Object (File)
Sat, Dec 14, 7:02 AM
Unknown Object (File)
Sat, Dec 14, 7:02 AM
Unknown Object (File)
Sat, Dec 14, 6:54 AM
Unknown Object (File)
Fri, Dec 13, 10:58 PM
Unknown Object (File)
Fri, Dec 13, 10:25 AM
Unknown Object (File)
Wed, Dec 4, 7:07 PM

Details

Summary

Depends on D4889

Prepare the put client to handle bidirectional stream operations.

Test Plan

Test plan in D4946

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

karol retitled this revision from [services] Backup - Connect to Blob - Prepare for bidi stream to [DRAFT] [services] Backup - Connect to Blob - Prepare for bidi stream.Aug 24 2022, 6:00 AM
karol edited the summary of this revision. (Show Details)
karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, varun, max.
tomek requested changes to this revision.Aug 29 2022, 4:24 AM
tomek added inline comments.
services/backup/blob_client/src/put_client.rs
1

What is the reason for this change?

22–25

These names are slightly confusing. Can we chose one approach and stick to it? (either tx and rx or transmitter and receiver)

72–76

Is there a way to lock just once and set these four values?

82–91

Could you explain what's going on here? It seems like we take a rx, read from it and assign it again. Why this assignment is needed?

This revision now requires changes to proceed.Aug 29 2022, 4:24 AM
services/backup/blob_client/src/put_client.rs
1

A leftover, it will be reverted, thx.

22–25

Sure, no problem.

72–76

It's fixed in D4974

82–91

It's a hack, I had to do this, because when I wanted to use rx without take, rust would complain that I couldn't use it as mut (IIRC because it's a struct member).

services/backup/blob_client/src/put_client.rs
12–16 ↗(On Diff #16071)

If having a sender and receiver is intrinsic (e.g. the existence of it requires these things to be true) to a bidiclient, then I don't think we should have option on each field. Instead we should use Option<BidiClient> to see if it's been initialized. If you did get Some(BidiClient), then you can assume that it has valid sender/receiver fields.

20–26 ↗(On Diff #16071)
30–43 ↗(On Diff #16071)

Little more concise way to express this, and avoids acquiring a lock twice

If you do the Option<BidiClient> comment I did above, then you would just write:

fn is_initialized() -> bool {
  CLIENT.lock().expect("access client").is_some()
}
67 ↗(On Diff #16071)

We should be using a logging utilities for this. Then we can control the noise level with RUST_LOG

72 ↗(On Diff #16071)

would be nice for the expect to be a bit more descriptive. If we do see failures, then just "access client" being copy pasted every doesn't help too much

@jon if you have that many comments, please, request changes.

services/backup/blob_client/src/put_client.rs
12–16 ↗(On Diff #16071)

I initially wanted to do it like this but it didn't work for some reason. I'll try to do this but wouldn't give it a high prio. https://linear.app/comm/issue/ENG-1716/try-using-option-higher-in-hierarchy

20–26 ↗(On Diff #16071)
30–43 ↗(On Diff #16071)

fixed in D4974

67 ↗(On Diff #16071)

I couldn't make it work, they simply will not show on the screen. https://linear.app/comm/issue/ENG-1717/use-tracing-utilities-instead-of-println

72 ↗(On Diff #16071)

I think in the error message there is a line number so that's helpful enough. Also, we're getting rid of expects and unwraps later in the stack.

Abandoning this stack in favor of the new one that starts @ D5002.

All the comments have been either addressed inline or in the new stack.