Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CommandListeners #1382

Closed
sokomishalov opened this issue Aug 6, 2020 · 6 comments
Closed

Add support for CommandListeners #1382

sokomishalov opened this issue Aug 6, 2020 · 6 comments
Labels
type: feature A new feature
Milestone

Comments

@sokomishalov
Copy link
Collaborator

sokomishalov commented Aug 6, 2020

It would be great to have a handler API of executing commands.
I think there are a lot of possible use cases.

For example, I want to log all executing commands (input and output).
Current logging is not very human-friendly, actually :)
My temporary solution looks like this, but I don't think it's quite elegant:
https://gist.github.com/SokoMishaLov/207c8082594685daa44cbd60127960a8

Thanks!

@sokomishalov sokomishalov added the type: feature A new feature label Aug 6, 2020
@mp911de
Copy link
Collaborator

mp911de commented Aug 10, 2020

Thanks. We could introduce an API along the lines of:

public interface CommandListener {
    /**
     * Listener for command started events.
     *
     * @param event the event
     */
    default void commandStarted(CommandStartedEvent event) {
    }

    /**
     * Listener for command completed events
     *
     * @param event the event
     */
    default void commandSucceeded(CommandSucceededEvent event) {
    }

    /**
     * Listener for command failure events
     *
     * @param event the event
     */
    default void commandFailed(CommandFailedEvent event) {
    }

}

that get notified upon command events from CommandHandler. Since support for metrics and tracing are configured on ClientResources, it makes sense to put command listeners there, too.

Command events would typically hold the command type, arguments and the output (for success) and the error/exception for the failed case.

We also need to consider that commands may get canceled (i.e. connection closed) from the endpoint implementation which would ultimately call commandFailed.

@sokomishalov
Copy link
Collaborator Author

That's almost exactly what I expected to see.
I think the command event should also hold context/correlation/whatever to correlate events.

@mp911de
Copy link
Collaborator

mp911de commented Sep 10, 2020

Do you have an example of how you would like to attach data to a command or how the API for the context should look like?

@sokomishalov sokomishalov changed the title Implement some kind of command handler API Implement some kind of handler/listener/proxy API Sep 14, 2020
@sokomishalov
Copy link
Collaborator Author

sokomishalov commented Sep 14, 2020

I think it should look like typical key-value storage.
I suppose that r2dbc-proxy (well-known to you btw) has a nice API for similar purposes.

https://github.com/r2dbc/r2dbc-proxy/blob/main/src/main/java/io/r2dbc/proxy/core/QueryExecutionInfo.java#L180
https://github.com/r2dbc/r2dbc-proxy/blob/main/src/main/java/io/r2dbc/proxy/core/ValueStore.java

sokomishalov added a commit to sokomishalov/lettuce that referenced this issue Nov 1, 2020
@mp911de mp911de added this to the 6.1 M1 milestone Nov 8, 2020
mp911de pushed a commit that referenced this issue Nov 8, 2020
mp911de added a commit that referenced this issue Nov 8, 2020
Move events from models.events to event.command. Move CommandListener configuration from ClientResources to AbstractRedisClient.addListener. Consider changed writer chain in RedisChannelHandler.setTimeout. Add tests for standalone and Redis Cluster operations. Remove generics from command events to simplify interaction and listener implementation.

Replace generic ExecutionException with ExceptionFactory usage.

Original pull request: #1424.
@mp911de mp911de changed the title Implement some kind of handler/listener/proxy API Add support for CommandListeners Nov 8, 2020
@mp911de mp911de closed this as completed Nov 8, 2020
@FitBBC
Copy link

FitBBC commented Apr 21, 2022

No place to configure CommandListener found in ClientResources

@mp911de
Copy link
Collaborator

mp911de commented Apr 21, 2022

Command listeners are configured via RedisClient/RedisClusterClient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

No branches or pull requests

3 participants