feat(client): support admin operations in Java client #5671
Conversation
| * <p>If a non-200 response is received from the server, the {@code CompletableFuture} will be | ||
| * failed. |
There was a problem hiding this comment.
might be worth adding some documentation as to why this is different than listStreams and listTopic - maybe (if I'm understand what you're saying correctly):
| * <p>If a non-200 response is received from the server, the {@code CompletableFuture} will be | |
| * failed. | |
| * <p>If the ksqlDb server receives a non-200 response is received from the kafka broker, the {@code CompletableFuture} will be | |
| * failed. |
There was a problem hiding this comment.
It's actually listStreams and listTables that were different from all the rest of the methods (if you look at the other Client methods, you'll see they all have the same note as listTopics), since if ksqlDB is operating correctly, those shouldn't ever fail. However, it occurs to me that those requests can still fail if, e.g., ksqlDB is unavailable, or because of misconfigurations in the client, so I've added the note to those methods as well.
| * Returns the name of this stream. | ||
| * | ||
| * @return stream name |
There was a problem hiding this comment.
nit, my personal preference is just to have @return the name of this stream as one line if it's a simple getter
| final CompletableFuture<List<StreamInfo>> cf = new CompletableFuture<>(); | ||
|
|
||
| makeRequest( | ||
| "/ksql", |
There was a problem hiding this comment.
nit: might want to add some constants for the endpoints
| import io.confluent.ksql.api.client.StreamInfo; | ||
| import java.util.Objects; | ||
|
|
||
| public class StreamInfoImpl implements StreamInfo { |
There was a problem hiding this comment.
generally worth having equals/hashcode/tostring on externally exposed classes so that people can leverage them if they want to
There was a problem hiding this comment.
To make sure I understand correctly: you're suggesting we add equals/hashcode/tostring methods to each of the implementation classes, not the interfaces, right? This makes sense to me, though I'm not sure whether it makes more sense for the toString method to say StreamInfoImpl{...} or StreamInfo{...} since the former is an implementation detail, but the latter is a bit misleading.
There was a problem hiding this comment.
Hmm, good point - I'm satisfied with StreamInfo because I dont' think we expect more than just this impl
There was a problem hiding this comment.
Sounds good. I'm going to merge this PR and then open a follow-up to add equals/hashcode/tostring to all the client interfaces, since none of the new interfaces have them (not just the ones in this PR).
There was a problem hiding this comment.
Here's the follow-up: #5681
| ) { | ||
| return new TypeSafeDiagnosingMatcher<StreamInfo>() { | ||
| @Override | ||
| protected boolean matchesSafely( |
There was a problem hiding this comment.
would we need this if we implemented .equals()?
There was a problem hiding this comment.
I think so, for two reasons:
- I think it's useful for the test to validate the StreamInfo methods (getName, getTopic, getFormat) explicitly, since they're external APIs, rather than simply perform a comparison.
- StreamInfoImpl is package-private so the test can't even create StreamInfo instances for comparison, without exposing StreamInfoImpl which doesn't seem great.
There was a problem hiding this comment.
ah I had missed the distinction between the impl and the interface
| * <p>If a non-200 response is received from the server, the {@code CompletableFuture} will be | ||
| * failed. |
There was a problem hiding this comment.
It's actually listStreams and listTables that were different from all the rest of the methods (if you look at the other Client methods, you'll see they all have the same note as listTopics), since if ksqlDB is operating correctly, those shouldn't ever fail. However, it occurs to me that those requests can still fail if, e.g., ksqlDB is unavailable, or because of misconfigurations in the client, so I've added the note to those methods as well.
| import io.confluent.ksql.api.client.StreamInfo; | ||
| import java.util.Objects; | ||
|
|
||
| public class StreamInfoImpl implements StreamInfo { |
There was a problem hiding this comment.
To make sure I understand correctly: you're suggesting we add equals/hashcode/tostring methods to each of the implementation classes, not the interfaces, right? This makes sense to me, though I'm not sure whether it makes more sense for the toString method to say StreamInfoImpl{...} or StreamInfo{...} since the former is an implementation detail, but the latter is a bit misleading.
| ) { | ||
| return new TypeSafeDiagnosingMatcher<StreamInfo>() { | ||
| @Override | ||
| protected boolean matchesSafely( |
There was a problem hiding this comment.
I think so, for two reasons:
- I think it's useful for the test to validate the StreamInfo methods (getName, getTopic, getFormat) explicitly, since they're external APIs, rather than simply perform a comparison.
- StreamInfoImpl is package-private so the test can't even create StreamInfo instances for comparison, without exposing StreamInfoImpl which doesn't seem great.
Description
Fixes #4286
Adds support for listing streams, tables, and topics to the Java client.
Docs will come in a follow-up PR. Javadocs on the new methods and interfaces are included in this one.
Testing done
Added unit and integration tests.
Reviewer checklist