Details

Reviewers

varun
• jon
bartek

Commits

rCOMMca943860dfbf: [tunnelbroker-client] Adding `publish_message` client function

Summary

This diff introduces a publish_message client function for the shared Tunnelbroker client library.
The function handles publishing message payload to the gRPC stream which is bind to the mpsc tx argument.

Linear task: ENG-2728

Test Plan

Keyserver service is successfully built.
The function is used in the following D6460 in a NAPI-RS bridge function

Diff Detail

Repository

rCOMM Comm

Branch

tb-add-client-publish

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Jan 17 2023, 6:58 AM

• max held this revision as a draft.

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 17 2023, 6:58 AM

• max mentioned this in D6277: [tunnelbroker-client] Creating a Cargo project file.Jan 17 2023, 6:59 AM

Harbormaster failed remote builds in B15401: Diff 21001!Jan 17 2023, 7:00 AM

Updating to use tx mspc to publish.

Harbormaster completed remote builds in B15805: Diff 21551.Jan 29 2023, 11:46 PM

• max added a parent revision: D6281: [tunnelbroker-client] Adding Tunnelbroker tonic client initialization function.Jan 30 2023, 12:05 AM

Updating on the parent changes.

Harbormaster completed remote builds in B15807: Diff 21553.Jan 30 2023, 12:29 AM

• max edited the summary of this revision. (Show Details)Jan 30 2023, 2:46 AM

• max edited the test plan for this revision. (Show Details)

• max added reviewers: varun, • jon.

• max added a child revision: D6283: [keyserver] Adding `tunnelbroker-client` to the keyserver napi-rs dependencies.

• max published this revision for review.Jan 30 2023, 2:48 AM

• max edited the summary of this revision. (Show Details)

• max mentioned this in D6460: [keyserver] Adding Tunnelbroker-client napi-rs publish class method.Jan 30 2023, 2:59 AM

• jon added inline comments.Jan 31 2023, 9:12 AM

shared/tunnelbroker-client/src/lib.rs
32–49 ↗	(On Diff #21553)	Since we await, do we really need to decorate the function with async? Also, I think we can just return the result of await (not sure if this will cause type issues).

• jon added a reviewer: bartek.Jan 31 2023, 9:13 AM

just one question inline

shared/tunnelbroker-client/src/lib.rs
8 ↗	(On Diff #21553)	it's kind of weird that we are calling this module `protobuff`, i think `protobuf` is the standard nomenclature
32–49 ↗	(On Diff #21553)	any function where you await requires the async keyword
40–44 ↗	(On Diff #21553)	why are we using a vector if there's only one message?

This revision now requires changes to proceed.Jan 31 2023, 9:13 PM

• max mentioned this in rCOMM127dbcc6f0ea: [tunnelbroker-client] Creating a Cargo project file.Feb 1 2023, 6:39 AM

• max planned changes to this revision.Feb 1 2023, 6:45 AM

• max mentioned this in D6281: [tunnelbroker-client] Adding Tunnelbroker tonic client initialization function.Feb 2 2023, 3:11 AM

Addressing comments.

• max marked 4 inline comments as done.Feb 2 2023, 3:52 AM

• max added inline comments.

shared/tunnelbroker-client/src/lib.rs
8 ↗	(On Diff #21553)	it's kind of weird that we are calling this module `protobuff`, i think `protobuf` is the standard nomenclature Makes sense to change it to a more specific name. That name came from the parent D6281, I've changed it to the `tunnelbroker_pb`.
32–49 ↗	(On Diff #21553)	Since we await, do we really need to decorate the function with async? Yes, we should decorate it with async because we are calling await inside. Also, I think we can just return the result of await (not sure if this will cause type issues). Unfortunately, this causes a type issue, that's why such construction is used here.
40–44 ↗	(On Diff #21553)	why are we using a vector if there's only one message? Makes sense to pass a vector to this function as it's a shared client library, despite that in our current scenario we will publish one message in a napi-rs bridge. I've changed it. Thanks @varun !