-
Notifications
You must be signed in to change notification settings - Fork 986
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 #1424
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1424 +/- ##
==========================================
Coverage 78.81% 78.82%
- Complexity 6206 6261 +55
==========================================
Files 461 468 +7
Lines 20741 21039 +298
Branches 2283 2325 +42
==========================================
+ Hits 16348 16584 +236
- Misses 3337 3388 +51
- Partials 1056 1067 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this issue. I left a few comments about topics that we should address.
Please also note that we should have copyright headers on each new class with 2020 as inception year.
src/main/java/io/lettuce/core/CompositeRedisCommandListener.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/models/events/CommandFailedEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/CompositeRedisCommandListener.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/models/events/CommandStartedEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/models/events/CommandSucceededEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/models/events/CommandSucceededEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/protocol/EventListenerCommand.java
Outdated
Show resolved
Hide resolved
4f7ad04
to
3d73717
Compare
Targetting this PR for 6.1 since 6.0 will be released in the next few days. |
3d73717
to
96677eb
Compare
Ok. Fixed |
e98c516
to
177eec7
Compare
177eec7
to
f968a3e
Compare
f968a3e
to
961eccf
Compare
I started working on merging this PR. It provides a lot of value, but the real design challenges surfaced when trying to come up with a configuration model. Listeners aren't tied to I also removed generics from the command events to make things simpler. Let's see how this goes. I also noticed that introducing The changes are exciting and the implementation makes a lot of sense. With the merge some shortcomings of the current implementation of |
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.
Thank you for your contribution. That's merged and polished now. |
Draft for #1382