Add first V3 cluster APIs (+ V3 plumbing). #624
Conversation
The APIs added are:
* GET /clusters -> returns the list of clusters known
* GET /clusters/{clusterId} -> returns the cluster with the given clusterId
|
|
||
| @Override | ||
| protected void configure() { | ||
| install(new KafkaModule(context)); |
There was a problem hiding this comment.
why is this registered via BackendsModule? what's the advantage of the extra level?
(note: i may well ask some stupid questions due to ignorance of the frameworks etc., this may well be one of them).
| @@ -65,6 +65,13 @@ | |||
| + " hostname is used"; | |||
| public static final String HOST_NAME_DEFAULT = ""; | |||
|
|
|||
| public static final String ADVERTISED_LISTENERS_CONFIG = "advertised.listeners"; | |||
There was a problem hiding this comment.
I have not spent any time thinking about the advantages of including absolute URLs in responses, but for your consideration, want to note that requiring this additional config seems like a material downside.
There was a problem hiding this comment.
The configuration is not required. See UrlFactoryImpl for how the absolute URLs are generated. This configuration is to allow for generating URLs using a LB.
kafka-rest/src/main/java/io/confluent/kafkarest/controllers/ClusterManagerImpl.java
Outdated
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/controllers/ClusterManagerImpl.java
Outdated
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/response/FakeAsyncResponse.java
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/response/FakeUrlFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks great to me overall. I checked approve, but wait for input from others before merging.
I'm thinking it might be worth getting strong java developer to look over this at this stage of the project (given you're just getting going), but from my point of view, it's all looking good (or great).
@mageshn - care to take a quick look at the direction REST Proxy is taking?
|
|
||
| @Test | ||
| public void listClusters_returnListWithOwnCluster() throws Exception { | ||
| expect(adminClient.describeCluster(anyObject())).andReturn(describeClusterResult); |
There was a problem hiding this comment.
Why do we specify in the test name - returnListWithOwnCluster ? Don't we simply list all the clusters ?
There was a problem hiding this comment.
So, there's catch currently which is that Kafka (and the REST Proxy) does not know about other clusters. It only knows about the Cluster pointed by the bootstrap.servers property. I created the API this way so that in the future, when/if we do have information about other clusters, we can return them in the list as well. But right now, listing clusters always returns only one cluster.
|
Trying to follow the PR, since it has quite a few changes, I thought of following through tests to begin with, also I am pretty new to REST, might come up with simple questions. I notice the existing repo has folders as |
|
The entities that are in |
|
|
||
| @GET | ||
| @Produces(Versions.JSON_API) | ||
| public void listBrokers(@PathParam("clusterId") String clusterId) { |
There was a problem hiding this comment.
I see in v2 we were using @PerformanceMetric, why don't we use it now ?
There was a problem hiding this comment.
We can add performance metrics to all endpoints later.
The APIs added are: