Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Java: API layer with RedisClient #737
Java: API layer with RedisClient #737
Changes from all commits
8312b0b
62836d8
9e95ce6
114ed94
de3c4f3
1a31d3b
2a9e6ff
75d2494
617ab3e
1e1842c
26dd364
3ae1f07
098122f
76b5cd7
8092b62
30a8d64
6f1e3da
818a704
8546d9f
31bc4c5
9b02892
1d18311
0c7d03b
572fc7b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
restore interrupt
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.
I'm not sure this is right.
see: https://github.com/orgs/Bit-Quill/projects/4/views/6?pane=issue&itemId=48063887
Ultimately, it would be best to throw a RedisException/ClosingException after interrupting on all existing responses.
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.
I think all exeptions are properly propagated from that async pipeline. They are wrapped by
ExectionException
. Please, re-check. Maybe you can remove this TODO and/or add a test.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.
We have a task to add exception handling. It would be 'nicer' if the API supported throwing RedisException, rather than an ExecutionException with a RedisException.
I'm pretty certain we can extend CompletableFutures to handle RedisException, in parallel with ExcutionExceptions and that would provide a better experience for our users.
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.
Also, tests are already included in the follow-up PR, that adds RedisExceptions.
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.
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.
Is it necessary to run this in a separate thread?
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.
why not
fireChannelReadComplete
?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.
Because that doesn't pass the message to other read handlers. That tells the read pipeline to complete, I believe (documentation on that method is somewhat lacking).