Feat: Add redis backend #813
Conversation
There was a problem hiding this comment.
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)
| @@ -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") { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
They should not be conflicting, the server is selected with the dsn url
| /** | ||
| Topic messages are handled as other messages with redis, we return nothing. | ||
| */ | ||
| async fn fetch_topic_messages( |
There was a problem hiding this comment.
Oof, yeah, no.
These need to return topic messages. Topics are messages that replace pending versions. (You will only ever get back one topic message, but you will get back all the prior versions of regular messages.)
What this does is throw away topic messages.
Basically, the integration tests are going to have a fit when they try running against this, so it needs to work correctly.
There was a problem hiding this comment.
Topic messages are replaced and only the last version is kept. See comment bellow
|
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 Regarding the tests, they all pass |
|
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
|
All the commits are signed |
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:
run_gauntlet) is a bit different for this: it adds 2 messages for the same topic and check only one is fetched.fetch_timestamp_messagesare the date of the record in ms, with bigtables it uses the message'ssortkey_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.connectionFixes #774