Page MenuHomePhabricator

[Tunnelbroker] Initial message persistence
ClosedPublic

Authored by jon on May 11 2023, 9:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Feb 11, 12:06 AM
Unknown Object (File)
Sat, Feb 10, 8:22 AM
Unknown Object (File)
Sat, Feb 10, 8:22 AM
Unknown Object (File)
Sat, Feb 10, 8:21 AM
Unknown Object (File)
Sat, Feb 10, 8:21 AM
Unknown Object (File)
Sat, Feb 10, 8:21 AM
Unknown Object (File)
Sat, Feb 10, 8:20 AM
Unknown Object (File)
Sat, Feb 10, 8:12 AM
Subscribers

Details

Summary

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.

https://linear.app/comm/issue/ENG-3822

Test Plan
nix develop

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

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

Diff Detail

Repository
rCOMM Comm
Branch
jonringer/tunnelbroker-dynamodb (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

services/commtest/tests/tunnelbroker_integration_test.rs
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.
services/commtest/tests/tunnelbroker_integration_test.rs
55 ↗(On Diff #26432)

Yea, meant to comment. Thanks :)

96 ↗(On Diff #26432)

Created https://linear.app/comm/issue/ENG-3940.

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.
services/tunnelbroker/src/database.rs
51 ↗(On Diff #26432)

so we can only persist one message per device id?

73 ↗(On Diff #26432)

retrieve

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

nvm misunderstood the persist_message fn. just a couple nits

services/tunnelbroker/src/database.rs
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

services/tunnelbroker/src/database.rs
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: https://github.com/CommE2E/comm/blob/a79d72de974f45a44eefc5c3047f0477179841aa/services/tunnelbroker/src/constants.rs#L26

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)

thanks

services/tunnelbroker/src/database.rs
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:
https://github.com/CommE2E/comm/blob/3967ccade4fdec485631a1d9d629030f831180ba/services/identity/src/database.rs#L381

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