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.)

If this is a topic message: zadd(msg_list_key) and zadd(exp_list_key) will replace their old entry in the sorted sets if one already exists and set(msg_key, message) will override it too

Exclude lower band from redis range

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.

jrconlin · 2025-01-10T22:01:31Z

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

+    ///
+    /// If [`limit`] = 0, we fetch all messages after [`timestamp`].
+    ///
+    /// This can return expired messages, following bigtables behavior


Yeah, but there's no real need to do that, and in fact, it's probably an anti-pattern we need to fix in bigtable. (There's a potential abuse vector that should be cut off.)

Ideally, messages should be dropped once they've been decisively handled (e.g. if they're ACK'd or if the NACK response specifies that the message is undeliverable. see #795 )

Expired messages are removed from redis, so it doesn't use memory for expired messages.

But the message id is not removed from the message lists when it expires.

We can't just remove expired messages from the result, because if the messages vec is empty (so when there are 10 expired messages), check_storage_advance stops.

So there are 3 main solutions:

Change check_storage_advance to check if messages.is_empty() && timestamp_has_changed() // to write, but I didn't want to change logic on other components

Writing a loop in fetch_timestamp_messages but this is counter productive to do yet another loop

Returning dummy expired events, that check_sotrage_advance will remove (this is what it does now)

Ideally, messages should be dropped once they've been decisively handled (e.g. if they're ACK'd or if the NACK response specifies that the message is undeliverable. see #795 )

remove_message do remove the message, so this is already the case :)

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

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 added 19 commits January 7, 2025 19:42

Update yanked futures-utils

b902543

Add redis backend

1152f7f

Add docker-compose to self host the project

2659043

Remove useless ignore()

7118d5c

Wipe expired messages

32098b3

Don't use unwrap()

7df5533

Move redis import under feature flag

f676ee7

Actually use Uaid and Chanid wrappers

e86452c

Change emoji for redis logs

4529e74

Clean up some comments

193defb

Use standard chidmessageid

1d34450

Rename docker-compose for redis

03bdbe8

Fix settings tests with redis

a8167cd

Use multiplexed connection with redis

12c741d

Remove unused dep

8a9b146

Set potentially different chidmessageid for tests notifs

e133a00

Remove unnecessary topic entry

83ebc3c

If this is a topic message: zadd(msg_list_key) and zadd(exp_list_key) will replace their old entry in the sorted sets if one already exists and set(msg_key, message) will override it too

Store MultiplexedConnection as expected by redis

4dfd29d

Fix loop when fetching pending messages

f2f6d03

Exclude lower band from redis range

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

jrconlin and others added 2 commits January 13, 2025 13:21

Merge branch 'master' into feat/redis

837a426

Fix comments

00866ac

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

jrconlin Jan 10, 2025

p1gp1g Jan 11, 2025 •

edited

Loading

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

Choose a reason for hiding this comment

p1gp1g Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

p1gp1g commented Dec 21, 2024 •

edited

Loading

p1gp1g Jan 11, 2025 •

edited

Loading