Page MenuHomePhabricator
Differential D8177

[Tunnelbroker] Add amqp client crate, add minimal usage
ClosedPublic
Actions

Authored by • jon on Jun 11 2023, 6:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 13, 8:12 AM
Unknown Object (File)
Tue, Jun 11, 4:45 PM
Unknown Object (File)
Mon, Jun 10, 7:23 PM
Unknown Object (File)
Fri, May 24, 2:29 PM
Unknown Object (File)
Fri, May 24, 2:29 PM
Unknown Object (File)
Fri, May 24, 2:29 PM
Unknown Object (File)
Fri, May 24, 10:11 AM
Unknown Object (File)
Thu, May 23, 4:23 PM
View All 51 Files
Subscribers
atul
tomek

Details

Reviewers
varun
bartek
ashoat
Commits
rCOMM1d6de4d07114: [Tunnelbroker] Add amqp client crate, add minimal usage
Summary

lapin seems to be the most commonly used. It is:

  • Still actively maintained
  • MIT licensed
  • Supports much of amqp v0.9.x

Part of https://linear.app/comm/issue/ENG-3716

Test Plan

N/A

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• jon created this revision.Jun 11 2023, 6:58 PM
Herald added a subscriber: tomek. · View Herald TranscriptJun 11 2023, 6:59 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 11 2023, 7:01 PM
Harbormaster failed remote builds in B20169: Diff 27628!
• jon added a child revision: D8178: [Tunnelbroker] Use rabbitmq for message delivery.Jun 11 2023, 7:52 PM
• jon requested review of this revision.Jun 12 2023, 7:43 AM
ashoat resigned from this revision.Jun 12 2023, 8:23 AM

New package looks good! Resigning so that one of the other reviews can take a look at the Rust

bartek accepted this revision.Jun 12 2023, 9:02 AM
bartek added inline comments.
services/tunnelbroker/src/amqp.rs
9 ↗(On Diff #27628)

What's your policy on returning Result vs panicking?

Nit: We might want to return anyhow::Result<Connection> and return error, e.g. using with_context.
Anyway, it doesn't matter in the end - doing await? in main() results in panic anyway - it just might be more flexible.

This revision is now accepted and ready to land.Jun 12 2023, 9:02 AM
atul added a subscriber: atul.Jun 12 2023, 9:32 AM

(Shellcheck CI job should pass after pulling in latest changes.)

• jon added inline comments.Jun 12 2023, 10:07 AM
services/tunnelbroker/src/amqp.rs
9 ↗(On Diff #27628)

A failure in connecting to amqp would be a critical failure. I'm fine with an early panic, and the executable exiting immediately in this case.

• jon updated this revision to Diff 27674.Jun 12 2023, 2:51 PM

Rebase on master

Harbormaster completed remote builds in B20202: Diff 27674.Jun 12 2023, 3:10 PM
bartek added inline comments.Jun 12 2023, 11:14 PM
services/tunnelbroker/src/amqp.rs
9 ↗(On Diff #27628)

Makes sense, thanks

• jon marked an inline comment as done.Jun 15 2023, 10:56 AM
Closed by commit rCOMM1d6de4d07114: [Tunnelbroker] Add amqp client crate, add minimal usage (authored by • jon). · Explain WhyJun 19 2023, 8:13 PM
This revision was automatically updated to reflect the committed changes.
• jon added a commit: rCOMM1d6de4d07114: [Tunnelbroker] Add amqp client crate, add minimal usage.

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
1 line
src/
14 lines
6 lines
2 lines

Diff 27875

View Options

services/tunnelbroker/Cargo.lock

Loading...
View Options

services/tunnelbroker/Cargo.toml

Loading...
View Options

services/tunnelbroker/src/amqp.rs

Loading...
View Options

services/tunnelbroker/src/config.rs

Loading...
View Options

services/tunnelbroker/src/main.rs

Loading...