Redis Go Client Adds RESP3 Protocol Support - AI Conversation

sam 2024-03-15

Carefully review:

github.com/redis/go-redis

specify custom health check func via HeartbeatFn option

redis:master ← strobil:feature/custom-ring-shard-hc-func

opened 10:18PM - 13 Mar 24 UTC

strobil

+24 -5

The current implementation of the Redis ring does not support specifying custom …shard health check rules. We use a Redis shard to distribute the load across multiple Redis slave instances. However, if any shard encounters replication issues, the Redis ring continues to utilize this shard because it responds to ping requests successfully. Thus, the Redis ring does not remove the malfunctioning shard, even though it is inconsistent. This pull request proposes adding a **HeartbeatFn** option to the **RingOptions**, which can be configured during ring construction. This addition enables the definition of custom health check logic, such as checks based on the current replication status of the shard.

What does this do?
Is it implemented correctly?
Are there any changes you would recommend?

claude_3_opus_bot GitHub Helper 2024-03-15

GitHub pull request diff

redis/go-redis 2940

Based on reviewing the diff for pull request #2940 in the redis/go-redis repo, here are the key points:

What it does:

This PR introduces a new HeartbeatFn option in RingOptions which allows customizing the function used to check the liveness/health of each shard in the Redis ring.
It defines a defaultHeartbeatFn that uses the PING command to check if the shard is available. The shard is considered up if PING succeeds or returns a pool timeout error.
The heartbeat logic in the ringSharding.Heartbeat method is updated to use the configured HeartbeatFn instead of directly calling PING.

Implementation:

The changes look implemented correctly. The new HeartbeatFn option is added to RingOptions with proper documentation.
The defaultHeartbeatFn is defined as a package-level var for easy customization if needed.
The RingOptions.init method sets the HeartbeatFn to defaultHeartbeatFn if not provided by the user.
The heartbeat logic correctly uses the configured HeartbeatFn to determine the shard’s status.

Recommendations:

The changes look good overall and allow more flexibility in determining shard health.
One suggestion would be to consider exporting the defaultHeartbeatFn as DefaultHeartbeatFn so users can refer to it when providing custom functions. E.g:

HeartbeatFn: func(ctx, client) bool {
  // custom checks...
  return ring.DefaultHeartbeatFn(ctx, client)
}

The documentation could mention that the default uses PING and considers timeouts as the shard being available. This helps users understand the default behavior.

But in general, this PR looks good to me and adds a nice customization point for handling shard health checks in different environments/setups. The implementation follows Go idioms and doesn’t have any obvious issues from what I can see in the diff.