-
Notifications
You must be signed in to change notification settings - Fork 5
Rs3 range query support #435
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
Conversation
...-client/src/main/java/dev/responsive/kafka/internal/db/rs3/client/grpc/GrpcMessageQueue.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| interface Mapper<T> { |
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.
nit: maybe call this a Visitor instead? This looks like the classic visitor pattern to me
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.
Yeah, it's a visitor pattern, but I'm using it as a poor man's version of pattern matching to implement a map function. I thought using the Visitor name made its usage feel a little too vague. I don't feel strongly about it though..
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.
Up to you - I felt the opposite 😛 . I was a bit confused at first and had to look at how it was used but it would've felt obvious if it was named Visitor. But that could just be me. We had the Visitor pattern drilled into us working on ksql syntax trees 😄
rodesai
left a comment
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.
This looks great! My only question is about the queue implementation - why not just use the java BlockingQueue rather than rolling our own?
rodesai
left a comment
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.
LGTM assuming the answer to my question about poll/peek is yes
|
|
||
| public class GrpcMessageQueue<T> { | ||
| private final BlockingQueue<T> queue = new LinkedBlockingQueue<>(); | ||
| private final AtomicReference<T> next = new AtomicReference<>(); |
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.
does this need to be atomic? you can't do concurrent polls/peeks since taking the next item from the queue and setting next need to be synchronized
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.
Yeah, fair enough.
| } | ||
| } | ||
|
|
||
| T poll() { |
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.
multiple calls to poll/peek won't ever be done concurrently right?
Adds support for range queries over GRPC. We expose a new method
Rs3Client.rangewhich returns a blocking iterator. Internally in the iterator implementation, transient errors are caught and the operation is retried using the same exponential backoff we use for other operations.