Feat: Add redis backend #813

p1gp1g · 2024-12-19T10:32:20Z

Currently, autopush requires using Bigtable, there is no particular issue for a big, public server, but some may want to host their own server, or to do some tests locally. So this PR adds another kind of backend using Redis. It should work with redis forks too.

It mostly follow the behavior of bigtable with a few differences:

when saving a message with a topic, it simply removes the previous message for this topic. So topic messages and timestamp messages are processed the exact same way. Therefore the test (run_gauntlet) is a bit different for this: it adds 2 messages for the same topic and check only one is fetched.
Timestamps used and returned by fetch_timestamp_messages are the date of the record in ms, with bigtables it uses the message's sortkey_timestamp, in seconds.

I've also added a docker-compose file to run everything.

For the moment RedisClientImpl doesn't use connection pooling, but this should be easily implemented, it also provide the Command trait, returned by RedisClientImpl.connection

Fixes #774

jrconlin

Gotta admit, Redis is not really the DB that I would have picked for this, but sure, provided that the durable storage element is set, it should be able to handle light loads.

FWIW, I had done a version that used Postgres a long time ago. (If you go through the Changelog, you might find a reference to it.) Sadly, I burned that branch a while ago.

Still, thank you for this. There are a few things you'll need to do to fix it up.

The requirements are:

Please don't use .unwrap() (or if you do, please explain why.)
We actually do need to support topic messages.
(Make sure that all unit and integration tests pass)

autoendpoint/src/server.rs

jrconlin · 2024-12-20T18:10:16Z

autopush-common/build.rs

@@ -1,5 +1,5 @@
 pub fn main() {
-    if !cfg!(feature = "bigtable") {
-        panic!("No database defined! Please compile with `features=bigtable`");
+    if !cfg!(feature = "bigtable") && !cfg!(feature = "redis") {


bigtable and redis are conflicting feature flags, no?

If you like, we do a conditional check in syncstorage-rs to prevent folk from accidentally specifying both flags.

They should not be conflicting, the server is selected with the dsn url

Right, the actual database selection is non-conflicting, but the compilation is not. I don't know of a situation where someone might want to have both Bigtable and Redis support enabled on the back end at the same time, considering that there's only one database you can specify or use.

I agree there is no use case when both are used, but I can see some examples for compiling both. For instance if a generic container image is published/used, or one do a build that can be used in different environments, (dev/tests with redis, integration/prod with bigtable)

autopush-common/src/db/redis/redis_client/mod.rs

autopush-common/src/notification.rs

docker-compose.yml

p1gp1g · 2024-12-21T14:05:39Z

Thank you for the review, I've addressed most of the comments.

I went to redis, because this is also what I used for Uppush, which also work as a webpush server. I've seen in the issue comments you had a version with Postgres.

Regarding the topics, this implementation supports topic, but in another way. See the tests: we store 2 messages with the same topic and fetch a single one (if I set topic to None, it fails)

        // We store 2 messages, with a single topic
        let test_notification_0 = crate::db::Notification {
            channel_id: topic_chid,
            version: "version0".to_owned(),
            ttl: 300,
            topic: Some("topic".to_owned()),
            timestamp,
            data: Some(test_data.clone()),
            sortkey_timestamp: Some(sort_key),
            ..Default::default()
        };
        assert!(client
            .save_message(&uaid, test_notification_0.clone())
            .await
            .is_ok());

        let test_notification = crate::db::Notification {
            timestamp: now(),
            version: "version1".to_owned(),
            sortkey_timestamp: Some(sort_key + 10),
            ..test_notification_0
        };

        assert!(client
            .save_message(&uaid, test_notification.clone())
            .await
            .is_ok());

        let mut fetched = client.fetch_timestamp_messages(&uaid, None, 999).await?;
        assert_eq!(fetched.messages.len(), 1);

By the way, using the current chidmessageid allows to do this automatically, the topic isn't necessary: ddc8a46

Regarding the tests, they all pass cargo test --no-default-features --features redis

jrconlin · 2025-01-07T17:15:44Z

Sigh, just like the other PR, I'm afraid we can't accept this PR with unsigned commits. Please feel free to resubmit this with signed commits.

(This is a policy put in place by our Security Operations team.)

p1gp1g · 2025-01-07T18:44:54Z

All the commits are signed

jrconlin · 2025-01-02T19:35:00Z

autopush-common/build.rs

@@ -1,5 +1,5 @@
 pub fn main() {
-    if !cfg!(feature = "bigtable") {
-        panic!("No database defined! Please compile with `features=bigtable`");
+    if !cfg!(feature = "bigtable") && !cfg!(feature = "redis") {


Right, the actual database selection is non-conflicting, but the compilation is not. I don't know of a situation where someone might want to have both Bigtable and Redis support enabled on the back end at the same time, considering that there's only one database you can specify or use.

jrconlin · 2025-01-10T21:44:39Z

autopush-common/src/db/redis/redis_client/mod.rs

+
+mod error;
+
+use super::RedisDbSettings;


nit: (feel free to ignore) we don't tend to use super a lot because it can make refactoring surprisingly weird. Instead we usually root off of crate like above.

so this would be:

use crate::db::redis::RedisDbSettings;

autopush-common/src/db/redis/redis_client/mod.rs

jrconlin · 2025-01-10T21:51:44Z

autopush-common/src/db/redis/redis_client/mod.rs

+        Ok(channels)
+    }
+
+    /// Delete the channel. Does not delete its associated pending messages.


As I note above you might be the exception here and actually want to delete data when the channel is dropped.

jrconlin · 2025-01-10T21:55:43Z

autopush-common/src/db/redis/redis_client/mod.rs

+            conn: Arc::new(Mutex::new(None)),
+            settings: db_settings,
+            metrics,
+            redis_opts: SetOptions::default().with_expiration(SetExpiry::EX(MAX_ROUTER_TTL)),


So, fun fact: Bigtable and dynamodb can make writes kind of expensive, so we instead rely on TTLs for data to get cleaned up. Redis doesn't really have that limit, so while having an expiration definitely makes sense, feel free to drop values as they're no longer required (e.g. once you get an ACK, when the ChannelID or UserID is dropped, etc.)
Honestly, this is probably a lot more important in the context of Redis, since memory is more a problem than disk space.

autopush-common/src/db/redis/redis_client/mod.rs

wrenix · 2025-01-16T10:53:53Z

Is there any healthycheck? It looks like, that autoconnect does not work if the redis is not ready on startup (and afterwards no reconnect happens and my query of /health think everything is fine)

p1gp1g · 2025-01-16T12:47:20Z

Is there any healthycheck? It looks like, that autoconnect does not work if the redis is not ready on startup (and afterwards no reconnect happens and my query of /health think everything is fine)

I thought getting the connection was enough. I've added a ping to the healthcheck (which was added last week to the redis crate 👍 )

p1gp1g · 2025-02-10T17:56:39Z

I have rebased this branch too

jrconlin · 2025-02-10T19:01:42Z

Sigh. Just noticed that these commits aren't signed. We require all commits to be signed before we can accept them. (I've tried to make that very clear in the docs and PR templates, but I guess there's still work to do)

Sadly, that means we can't accept this PR as is (even one unsigned commit corrupts things.)

You can either close this PR and resubmit it or I can submit it on your behalf, but I'll need one of the other core folk to sign off on it.

p1gp1g · 2025-02-11T12:56:53Z

I have squashed the branch and signed the commit, let me know if you prefer keep the different commits for the review

p1gp1g force-pushed the feat/redis branch from 7404e34 to 27b802d Compare December 19, 2024 10:59

b4handjr requested review from jrconlin and pjenvey December 20, 2024 17:44

jrconlin requested changes Dec 20, 2024

View reviewed changes

p1gp1g force-pushed the feat/redis branch from 837ee58 to f2f6d03 Compare January 7, 2025 18:44

jrconlin reviewed Jan 10, 2025

View reviewed changes

p1gp1g force-pushed the feat/redis branch from f82bc8a to d6a998e Compare February 10, 2025 17:55

Add redis backend

3aa04fe

p1gp1g force-pushed the feat/redis branch from d6a998e to 3aa04fe Compare February 11, 2025 12:54

jrconlin self-requested a review February 11, 2025 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add redis backend #813

Feat: Add redis backend #813

p1gp1g commented Dec 19, 2024

jrconlin left a comment

jrconlin Dec 20, 2024

p1gp1g Dec 21, 2024

jrconlin Jan 2, 2025

p1gp1g Jan 11, 2025

p1gp1g commented Dec 21, 2024 •

edited

Loading

jrconlin commented Jan 7, 2025

p1gp1g commented Jan 7, 2025

jrconlin Jan 2, 2025

jrconlin Jan 10, 2025

jrconlin Jan 10, 2025

jrconlin Jan 10, 2025

wrenix commented Jan 16, 2025

p1gp1g commented Jan 16, 2025

p1gp1g commented Feb 10, 2025

jrconlin commented Feb 10, 2025

p1gp1g commented Feb 11, 2025

Feat: Add redis backend #813

Are you sure you want to change the base?

Feat: Add redis backend #813

Conversation

p1gp1g commented Dec 19, 2024

jrconlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p1gp1g commented Dec 21, 2024 • edited Loading

jrconlin commented Jan 7, 2025

p1gp1g commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrenix commented Jan 16, 2025

p1gp1g commented Jan 16, 2025

p1gp1g commented Feb 10, 2025

jrconlin commented Feb 10, 2025

p1gp1g commented Feb 11, 2025

p1gp1g commented Dec 21, 2024 •

edited

Loading