Page MenuHomePhabricator

[Tunnelbroker] Initial message persistence

Authored by jon on May 11 2023, 9:34 PM.
Referenced Files
F1867943: D7799.diff
Sun, May 26, 4:54 PM
Unknown Object (File)
Tue, May 21, 7:32 PM
Unknown Object (File)
Sat, May 18, 1:35 PM
Unknown Object (File)
Mon, May 13, 10:58 AM
Unknown Object (File)
Sun, May 5, 3:24 AM
Unknown Object (File)
Sat, May 4, 6:25 AM
Unknown Object (File)
Thu, May 2, 11:26 PM
Unknown Object (File)
Thu, May 2, 11:23 AM



Allow for messages to be enqueued to dynamoDB when the
device is not available. Also, deliver messages once a connection with
said device becomes available.

Test Plan
nix develop

# init localstack and run terraform
comm-dev services start
(cd services/terraform && ./

(cd services/tunnelbroker && cargo run &)
(cd services/commtest && cargo test --test tunnelbroker_integration_test)

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

Looks good to me but I'd also let @varun take a look

55 ↗(On Diff #26432)

For future reviewers: This unwrap is refactored in the next diff 😉

96 ↗(On Diff #26432)

Just a general note:
We eventually might want to store the tested service hostnames and ports in a separate, easily configurable place. If you have some ideas already, maybe we could discuss them in some Linear task, I'll be dealing with the same problem when designing blob service HTTP tests very soon so maybe we can do it the right way 😄

(for blob and backup we use the env variable for port number but this isn't clean and flexible enough)

jon added inline comments.
55 ↗(On Diff #26432)

Yea, meant to comment. Thanks :)

96 ↗(On Diff #26432)


But I agree, it would nice to create a paradigm for configuring this.

varun requested changes to this revision.May 30 2023, 1:50 PM
varun added inline comments.
51 ↗(On Diff #26432)

so we can only persist one message per device id?

73 ↗(On Diff #26432)


This revision now requires changes to proceed.May 30 2023, 1:50 PM

nvm misunderstood the persist_message fn. just a couple nits

66 ↗(On Diff #26432)

nvm, i didn't realize that this is the sort key. would be helpful to label it similar to how you've labeled PARTITION_KEY two lines above

This revision is now accepted and ready to land.May 30 2023, 2:00 PM
jon marked 5 inline comments as done.

Address feedback

51 ↗(On Diff #26432)

no, created_at is a sort key. So there can be many.

66 ↗(On Diff #26432)

I do have it as an alias:

I chose to use the CREATED_AT since it more aligns with the business logic, and is easier for me to follow. I would also prefer to use DEVICE_ID, but we already established the usage of PARTITION_KEY in identity service.

If you're adamant about preferring the current schema structure over field names, I can change it.

73 ↗(On Diff #26432)


66 ↗(On Diff #26432)

we already established the usage of PARTITION_KEY in identity service

we've also established the usage of SORT_KEY in the identity service:

i think it's fine to use 1) DEVICE_ID and CREATED_AT or 2) PARTITION_KEY and SORT_KEY, but as currently written, it's not clear that the table has a composite primary key