Custom Redis JSON Function Support for READ Intent Operations

[bdchatham]

Hi there,

I am currently building a caching layer which uses Redis JSON as the value type. I am also intending to use the LOWEST_LATENCY ReadFrom field to help reduce network latency. I am using a custom Command, JSON.GET, with dispatch on my AdvancedRedisClusterCommands instance.

When I ran a basic test querying random keys, I noticed all requests are going to the primary node. After turning on debug logs, I noticed many logs lines like: io.lettuce.core.cluster.PooledClusterConnectionProvider: getConnection(WRITE, 6708). 6708 in this case represents a slot. Clearly, my command is being interpreted as a write and therefore, must go to the primary.

Digging deeper, I noticed a few things. 1) unless I have misread something, the ReadIntent logic skips ClusterCommands and defaults them to Writes; 2) even if a specific connection to a node is used, it uses a static set of Command names to determine if a command has ReadIntent. Of course, my command is not in that set.

Is anyone aware of a way to execute my command with READ intent while also leveraging the LOWEST_LATENCY functionality in cluster mode?

Sorry if any of this is inaccurate or unclear. I've been at this for awhile. Thanks in advance.

[bdchatham]

To add more to this investigation, I found that after changing my same code to use .get(key) instead of a custom command via dispatch(...) that the logs show LOWEST_LATENCY is being respected. Below is an example log:

io.lettuce.core.cluster.PooledClusterConnectionProvider: getConnection(READ, 15413)

[JimB123]

@mp911de Looking into the code, it seems that ReadOnlyCommands.java contains the hard-coded list of read-only commands. Given that the methods are static and the data is private static final, there's no way to inject additional (module-based) commands.

It's also inconvenient that the isReadOnlyCommand() method takes a ProtocolKeyword (interface) which is good, but the data structure itself is a Set<CommandType> (where CommandType implements ProtocolKeyword). So, while it's possible to test if an arbitrary ProtocolKeyword is contained in the set, only actual CommandType objects can actually exist in the set.

[JimB123]

@mp911de I'd be happy to author a PR in this area. Do you have any suggestions/direction on a method of solution which would be acceptable to the project? One approach might be to add a Set<ProtocolKeyword> which would be a superset to READ_ONLY_COMMANDS and expose a method for additions. We'd need to keep the existing Set<CommandType> in order to maintain backward compatibility for the public method getReadOnlyCommands().

[mp911de]

ReadOnlyCommands are currently internal utilities; we're free to change these. Over time, we've seen requests to be able to affect command routing and include module/custom commands.

We just didn't had a good place to put these utilities in as there's no common package that contains code for multi-node operations.

In any case, io.lettuce.core.protocol would be good to host such functionality as ConnectionIntent already is part of the package.

Something along the lines of an interface, that returns the ConnectionIntent from a RedisCommand (to give maximum context to the strategy implementation) would make sense here. ClientOptions would be a good place to hold the implementation and provide it to Master/Replica and Redis Cluster.

With Redis Cluster working slightly different regarding pub/sub, we would pre-initialize ClusterClientOptions with a slightly different set of commands types.

The entire mechanism needs to be composable so that if one implementation cannot determine whether it is a read or a write command, then the next component in the chain would try to figure it out until we fall back to some default.

With all these aspects in place, we should be able to develop a neat design.

Let me know whether this helps.

[JimB123]

@mp911de I've dug into the code, and this is what I've found...

• Read-only commands are listed in a static class ReadOnlyCommands. This class exists in 2 versions (masterreplica & cluster). As you have previously indicated, this class is not part of the API, nor is it appropriate to add custom module values into this object.

• Moving on to where the read-only determination is made, this is performed in the getIntent() method of both ClusterDistributionChannelWriter & MasterReplicaChannelWriter - this is the logic which needs to be extended/altered.

• You've mentioned that the ClientOptions would be a good place to configure this, and I agree. I'm envisioning a Set<ProtocolKeyword> which can (optionally) be added as an option. This would exist in ClientOptions and would also be available to the subclass ClusterClientOptions.

• Next, we need to make the ClientOptions available to the ClusterDistributionChannelWriter & MasterReplicaChannelWriter.

  • The constructor for ClusterDistributionChannelWriter already requires a ClientOptions parameter - however that value is just used transiently within the contstructor. I believe we will now want to maintain a reference to the ClientOptions so that we can use it from the getIntent() method.
  • The constructor for MasterReplicaChannelWriter does not currently have access to a ClientOptions. I see no simple mechanism to access the options given the existing interface. I suggest that the constructor will need to be modified to include ClientOptions similarly to the constructor for ClusterDistributionChannelWriter.

• The getIntent() functions are currently static. The static designation will need to be removed so that they can access the new reference to ClientOptions. Only the unit test cases use these functions in a static manner. These test cases will need to be modified to create a the actual channel writer rather than directly calling the static method.

• The getIntent() logic will need to be changed to check if a command is read-only by checking if the command is listed in ReadOnlyCommands OR within the ClientOptions.

ALTERNATIVELY, we could, by default, set the contents of the read-only command set in ClientOptions to the values in ReadOnlyCommands, and then we could add to it if ClientOptions is updated with additional commands. The setting logic would be a bit more complex, but run-time lookups would only have to consult one set of commands. I'm not sure that the additional complexity is justified.

[mp911de]

MasterReplicaChannelWriter is package-private, so we're good to change its constructor. Methods that create MasterReplicaChannelWriter instances have access to RedisClient and we can obtain ClientOptions from there.

With our rewrite, we would not check against ReadOnlyCommands directly but rather use ReadOnlyCommands to create a default instance of the utility function to discover whether the command is a read-only one.