Page MenuHomePhabricator

[Tunnelbroker] implement Tunnelbroker confirmation for sender
ClosedPublic

Authored by kamil on Oct 12 2023, 7:19 AM.
Tags
None
Referenced Files
F3522728: D9467.id32134.diff
Mon, Dec 23, 7:30 AM
F3522639: D9467.id31966.diff
Mon, Dec 23, 7:18 AM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:38 PM
Subscribers

Details

Summary

Issue: ENG-5149.

Protocol diagram: notion doc.

Depends on D9463

Test Plan

Run tests, make sure tests actually coding use cases by looking at Tunnelbroker logs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Oct 12 2023, 8:16 AM
services/tunnelbroker/src/websockets/mod.rs
199 ↗(On Diff #31966)

Should we unwrap here? Is it going to be handled gracefully?

services/tunnelbroker/src/websockets/session.rs
184–188 ↗(On Diff #31966)

Why are we deleting a message if queue publish doesn't work?

194–220 ↗(On Diff #31966)

If we are ignoring values of Err(_) (first match) and other message types (second match) we could just use let else.

Separately I'm not sure where you are planning to put the "send message batch logic" but it might make sense to return MessageSentStatus here instead of SendConfirmation

305 ↗(On Diff #31966)

Nit: imo it's better to specify the unit value so the reader doesn't have to think what value are we ignoring

address review

services/tunnelbroker/src/websockets/mod.rs
199 ↗(On Diff #31966)

changed to break

services/tunnelbroker/src/websockets/session.rs
184–188 ↗(On Diff #31966)

The publish result is still Ok when recipient is not connected, and messages are persisted in the correct order. This will fail when things like connection with the RMQ broker is broken. I think it's okay then to inform the client that something went wrong, but I don't feel strongly, we can discuss this if you have a different point of view.

194–220 ↗(On Diff #31966)

If we are ignoring values of Err(_) (first match) and other message types (second match) we could just use let else.

right, thanks

Separately I'm not sure where you are planning to put the "send message batch logic" but it might make sense to return MessageSentStatus here instead of SendConfirmation

makes sense

This revision is now accepted and ready to land.Oct 18 2023, 2:56 AM