ref(relay): migrate redis to async for projectconfig #4284

Litarnus · 2024-11-22T15:43:48Z

Replaces all sync redis calls for projectconfig with the async equivalent.

relay-redis/src/real.rs

jjbayer · 2024-11-25T17:07:13Z

Cargo.toml

@@ -68,6 +68,7 @@ axum-extra = "0.9.3"
 axum-server = "0.7.1"
 arc-swap = "1.7.1"
 backoff = "0.4.0"
+bb8-redis = "0.17.0"


Haven't done a full review, but what's the new dependency for? Doesn't redis-rs support async out of the box? https://github.com/redis-rs/redis-rs?tab=readme-ov-file#async-support

This is the connection pool, the async equivalent of r2d2

Litarnus · 2024-11-26T08:41:42Z

relay-redis/src/real.rs

+                timeout(
+                    Duration::from_secs(options.read_timeout),
+                    cmd.query_async(&mut *conn),
+                )


Since async redis doesn't seem to have set_read_timeout/set_write_timeout anymore, I wrapped the query call in a tokio timeout. From what I understand both write and read timeouts are applied for every command while sending the command to redis and waiting for the reply respectively.
If that's true I think it would be better to use timeout(read_timeout + write_timeout, ...) to allow for the same total waiting as before

Isn't this the same as set_response_timeout on the multiplexed connection or better, set connection and response timeouts on AsyncConnectionConfig and with get_multiplexed_async_connection_with_config this should do the right thing? I think we should just rely on that.

If that's true I think it would be better to use timeout(read_timeout + write_timeout, ...) to allow for the same total waiting as before.

This still has different semantics. Socket read/write is independent from total duration.

You're right, set_response_timeout exists for the multiplexed connection obtained from the pool for redis in single server mode. The cluster connection does not expose this, however, I noticed that the ClusterClientBuilder allows us to set the response timeout.

connection_timeout is already handled by the pool builder.

The original thought still remains unclear for me, does it make sense to set the timeout to read + write? Or what would be the proper timeout in this case without changing the config structure?

The original thought still remains unclear for me, does it make sense to set the timeout to read + write?

I think just limiting it to read is fine. Let's just document the behaviour and change the config once all clients are async.

Dav1dde · 2024-11-26T09:37:57Z

relay-redis/src/real.rs

+    type Connection = ClusterConnection;
+    type Error = redis::RedisError;
+
+    fn connect<'life0, 'async_trait>(


We can just use async_trait here, it's in the dependency tree anyways.

Seems like I misunderstood what the dev guide says about async_trait. I thought it suggests to not use the crate, but after reading it again it does not mention this at all.

I will update accordingly

Note, that link is also slightly outdated, Rust traits can now be async fn.

Dav1dde · 2024-11-26T09:38:29Z

relay-redis/src/real.rs

+        'life0: 'async_trait,
+        'life1: 'async_trait,
+    {
+        Box::pin(async move { redis::cmd("PING").query_async(conn).await })


Should also check the response if it is PONG.

See also: https://docs.rs/bb8-redis/0.17.0/src/bb8_redis/lib.rs.html#72-75

Dav1dde · 2024-11-26T09:45:19Z

relay-redis/src/real.rs

+                timeout(
+                    Duration::from_secs(options.read_timeout),
+                    cmd.query_async(&mut *conn),
+                )


Isn't this the same as set_response_timeout on the multiplexed connection or better, set connection and response timeouts on AsyncConnectionConfig and with get_multiplexed_async_connection_with_config this should do the right thing? I think we should just rely on that.

If that's true I think it would be better to use timeout(read_timeout + write_timeout, ...) to allow for the same total waiting as before.

This still has different semantics. Socket read/write is independent from total duration.

Dav1dde · 2024-11-26T09:46:23Z

relay-redis/src/real.rs

+    /// Forwards the [`Script`] to the pool and loads it asynchronously.
+    /// Returns the SHA1 hash of it.
+    pub async fn load_async(&self, script: &Script) -> Result<String, RedisError> {
+        match self {
+            Self::Cluster(pool, _options) => {
+                let mut conn = pool.get().await.map_err(RedisError::AsyncPool)?;
+                script
+                    .prepare_invoke()
+                    .load_async(&mut *conn)
+                    .await
+                    .map_err(RedisError::Redis)
+            }
+            Self::Single(pool, _options) => {
+                let mut conn = pool.get().await.map_err(RedisError::AsyncPool)?;
+                script
+                    .prepare_invoke()
+                    .load_async(&mut *conn)
+                    .await
+                    .map_err(RedisError::Redis)
+            }
+        }
+    }


This is only necessary for the multi-write connection stuff, we don't support it for Async, might as well kill all of it.

This should also make your create_redis_pools fn sync again making a bunch of stuff easier.

Nice, I'm happy to delete it

Dav1dde · 2024-11-26T09:51:03Z

relay-redis/src/real.rs

+
+    /// Creates a new [`AsyncRedisPool`] backed by a single redis server.
+    pub async fn single(server: &str, opts: &RedisConfigOptions) -> Result<Self, RedisError> {
+        let manager = bb8_redis::RedisConnectionManager::new(server).map_err(RedisError::Redis)?;


If we want a custom connection config, I don't think the manager allows us to use get_multiplexed_async_connection_with_config. Might need our own one and can think about contributing that upstream separately.

I will look into this, seems like a good idea to make the connection manager set the proper config

I added another custom ConnectionManager that allows us to use the connection with custom config. It feels more consistent between cluster and non cluster pools because the timeout is set once on pool creation instead of setting it on the connection directly. Thank you for the idea!

…imeouts

Dav1dde · 2024-11-26T13:20:27Z

relay-redis/src/real.rs

+    }
+}
+
+#[axum::async_trait]


Let's add the async-trait crate directly and not use a re-export, also means the Redis crate does not depend on a web framework.

Dav1dde · 2024-11-26T13:22:22Z

relay-server/src/service.rs

+            // Based on the assumption that the first config is the
+            // primary config for MultiWrite configurations
+            let primary_config = configs.iter().next().ok_or(RedisError::Configuration)?;
+            Box::pin(create_async_pool(primary_config)).await


If we don't support it we should return an error.

I'm inclined to leave this in because projectconfigs used to work before with a multiwrite config and could now produce errors, especially when using the Unified config. So I think technically that would be a breaking change to previous versions.

Now it's a breaking change which is silent. The config may not fail, but it does something different.

On that note, this configuration before already did not make sense and was always experimental with a limited lifetime.

I don't think it's doing something different to an outside observer. It would always perform the commands for secondary configs in the background ignoring the result. Since it doesn't perform write operations for projectconfigs this just means that less work is done.

On that note, this configuration before already did not make sense and was always experimental with a limited lifetime.

I see, in this case I agree it is better to return an error and fix the config where needed

Dav1dde · 2024-11-26T13:23:41Z

relay-redis/src/real.rs

+    pub async fn single(server: &str, opts: &RedisConfigOptions) -> Result<Self, RedisError> {
+        let client = Client::open(server).map_err(RedisError::Redis)?;
+        let config = AsyncConnectionConfig::new()
+            .set_response_timeout(Duration::from_secs(opts.read_timeout));


Any reason you don't set the connect timeout?

If this is because it's redundant with the timeout specified on the pool, then we can probably use CustomizeConnection to set the remaining response timeout (we don't need a custom manager either).

Would also be good to have this as a comment somewhere.

Yes it's because of redundancy. I wanted to have as much config as possible there because it works the same for single and cluster redis.

I can switch to CustomizeConnection though and document the decision

Up to you, from just glancing over code and docs, it seems like CustomizeConnection is the easier way to do it.

Dav1dde · 2024-11-26T13:24:25Z

relay-server/src/service.rs

    match configs {
        RedisPoolConfigs::Unified(pool) => {
+            let async_pool = create_async_pool(&pool).await?;


Suggested change

let async_pool = create_async_pool(&pool).await?;

let project_configs = create_async_pool(&pool).await?;

Dav1dde · 2024-11-26T13:24:34Z

relay-server/src/service.rs

            let pool = create_redis_pool(pool)?;
            Ok(RedisPools {
-                project_configs: pool.clone(),
+                project_configs: async_pool,


Suggested change

project_configs: async_pool,

project_configs,

relay-server/src/service.rs

Dav1dde · 2024-11-26T13:29:45Z

relay-redis/src/real.rs

+    /// Creates a new [`AsyncRedisPool`] backed by a single redis server.
+    /// It will set the `response_timeout` to the provided `read_timeout`.
+    /// `write_timeout` is not used at all in the async pool


Suggested change

/// Creates a new [`AsyncRedisPool`] backed by a single redis server.

/// It will set the `response_timeout` to the provided `read_timeout`.

/// `write_timeout` is not used at all in the async pool

/// Creates a new [`AsyncRedisPool`] connecting to a single Redis server.

///

/// It will set the `response_timeout` to the provided `read_timeout`,

/// `write_timeout` is not used at all in the async pool.

Dav1dde · 2024-11-26T13:29:59Z

relay-redis/src/real.rs

+    /// Creates a new cluster based [`AsyncRedisPool`].
+    /// It will set the `response_timeout` to the provided `read_timeout`.
+    /// `write_timeout` is not used at all in the async pool


Suggested change

/// Creates a new cluster based [`AsyncRedisPool`].

/// It will set the `response_timeout` to the provided `read_timeout`.

/// `write_timeout` is not used at all in the async pool

/// Creates a new cluster based [`AsyncRedisPool`].

///

/// It will set the `response_timeout` to the provided `read_timeout`,

/// `write_timeout` is not used at all in the async pool.

Dav1dde · 2024-11-26T15:19:06Z

relay-server/src/service.rs

+            // Based on the assumption that the first config is the
+            // primary config for MultiWrite configurations
+            let primary_config = configs.iter().next().ok_or(RedisError::Configuration)?;
+            Box::pin(create_async_pool(primary_config)).await


Now it's a breaking change which is silent. The config may not fail, but it does something different.

On that note, this configuration before already did not make sense and was always experimental with a limited lifetime.

# Conflicts: # CHANGELOG.md

Litarnus added 4 commits November 22, 2024 16:43

ref(relay): migrate redis to async for projectconfig

05c70de

ref(relay): fix lint and import

1b2b3ef

ref(relay): add noop implementation

dccb45b

ref(relay): create pool for multi write config

6fb30c1

Litarnus marked this pull request as ready for review November 25, 2024 14:30

Litarnus requested a review from a team as a code owner November 25, 2024 14:30

ref(relay): add timeout to async pool

19b5fba

loewenheim requested changes Nov 25, 2024

View reviewed changes

relay-redis/src/real.rs Show resolved Hide resolved

relay-redis/src/real.rs Show resolved Hide resolved

relay-redis/src/real.rs Show resolved Hide resolved

jjbayer reviewed Nov 25, 2024

View reviewed changes

Litarnus added 3 commits November 25, 2024 22:35

ref(relay): move async pool into pools struct

76394fa

Merge branch 'master' into martinl/project-config-async-redis

9088af7

docs(projectconfig): update docs

92a4e08

Litarnus requested a review from loewenheim November 26, 2024 08:19

Litarnus commented Nov 26, 2024

View reviewed changes

loewenheim approved these changes Nov 26, 2024

View reviewed changes

Dav1dde reviewed Nov 26, 2024

View reviewed changes

Litarnus added 2 commits November 26, 2024 13:34

ref(projectconfig): use timeouts provided by redis instead of tokio t…

d9bd2ba

…imeouts

ref(projectconfig): remove async script loading

6db1277

Dav1dde reviewed Nov 26, 2024

View reviewed changes

Litarnus added 2 commits November 26, 2024 15:46

ref(projectconfig): comments on PR

c857390

ref(projectconfig): lint

ac3a3b0

Dav1dde approved these changes Nov 26, 2024

View reviewed changes

Dav1dde assigned Litarnus Nov 26, 2024

Litarnus added 2 commits November 26, 2024 16:59

ref(projectconfig): return error on projectconfig multiwrite

b8d6946

Merge branch 'master' into martinl/project-config-async-redis

bef1ee3

# Conflicts: # CHANGELOG.md

Litarnus merged commit 6046f47 into master Nov 27, 2024
23 checks passed

Litarnus deleted the martinl/project-config-async-redis branch November 27, 2024 12:20

jjbayer mentioned this pull request Nov 27, 2024

Identify dependency update that causes transaction processing regression #4301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(relay): migrate redis to async for projectconfig #4284

ref(relay): migrate redis to async for projectconfig #4284

Litarnus commented Nov 22, 2024 •

edited

Loading

jjbayer Nov 25, 2024

Litarnus Nov 25, 2024

Litarnus Nov 26, 2024 •

edited

Loading

Dav1dde Nov 26, 2024

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024 •

edited

Loading

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024 •

edited

Loading

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Litarnus Nov 26, 2024

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Litarnus Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

Dav1dde Nov 26, 2024

	let async_pool = create_async_pool(&pool).await?;
	let project_configs = create_async_pool(&pool).await?;

ref(relay): migrate redis to async for projectconfig #4284

ref(relay): migrate redis to async for projectconfig #4284

Conversation

Litarnus commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Litarnus Nov 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dav1dde Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dav1dde Nov 26, 2024 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Litarnus commented Nov 22, 2024 •

edited

Loading

Litarnus Nov 26, 2024 •

edited

Loading

Dav1dde Nov 26, 2024 •

edited

Loading

Dav1dde Nov 26, 2024 •

edited

Loading