You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/10 12:24:28 UTC
[GitHub] [kafka] mimaison opened a new pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
mimaison opened a new pull request #9726:
URL: https://github.com/apache/kafka/pull/9726
This PR adds a new REST endpoint to Connect: `GET /{connector}/tasks-config`, that returns the configuration of all tasks for the connector.
This changes is for [KIP-661](https://cwiki.apache.org/confluence/display/KAFKA/KIP-661%3A+Expose+task+configurations+in+Connect+REST+API)
Co-authored-by: Mickael Maison <mi...@gmail.com>
Co-authored-by: Oliver Dineen <di...@uk.ibm.com>
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r570545828
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##########
@@ -188,6 +188,16 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+ @GET
+ @Path("/{connector}/tasks-config")
+ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(final @PathParam("connector") String connector,
+ final @Context HttpHeaders headers,
Review comment:
You mean something like this?
```java
@GET
@Path("/{connector}/tasks-config")
public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(
final @PathParam("connector") String connector,
final @Context HttpHeaders headers,
final @QueryParam("forward") Boolean forward) throws Throwable {
FutureCallback<Map<ConnectorTaskId, Map<String, String>>> cb = new FutureCallback<>();
...
````
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r570505650
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##########
@@ -188,6 +188,16 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+ @GET
+ @Path("/{connector}/tasks-config")
+ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(final @PathParam("connector") String connector,
+ final @Context HttpHeaders headers,
Review comment:
nit: this alignment might be a bit off. We can follow the pattern of 2 tabs with first and last line on their own if that looks better I guess.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-760164790
Ping @rhauch @kkonstantine
The vote for this KIP passed, can you do a review? Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r570505650
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##########
@@ -188,6 +188,16 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+ @GET
+ @Path("/{connector}/tasks-config")
+ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(final @PathParam("connector") String connector,
+ final @Context HttpHeaders headers,
Review comment:
nit: this alignment might be a bit off. We can follow the pattern of 2 tabs with first and last line on their own if that looks better I guess.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##########
@@ -188,6 +188,16 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+ @GET
+ @Path("/{connector}/tasks-config")
+ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(final @PathParam("connector") String connector,
+ final @Context HttpHeaders headers,
Review comment:
That's right 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-742697895
@rhauch @kkonstantine can you take a look? Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r570577845
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##########
@@ -188,6 +188,16 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+ @GET
+ @Path("/{connector}/tasks-config")
+ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(final @PathParam("connector") String connector,
+ final @Context HttpHeaders headers,
Review comment:
That's right 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine merged pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API (KIP-661)
Posted by GitBox <gi...@apache.org>.
kkonstantine merged pull request #9726:
URL: https://github.com/apache/kafka/pull/9726
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-773706451
One failure on an unrelated flaky test
Merging to coordinate merge with another PR. Thanks @mimaison !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-773706451
One failure on an unrelated flaky test
Merging to coordinate merge with another PR. Thanks @mimaison !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine merged pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API (KIP-661)
Posted by GitBox <gi...@apache.org>.
kkonstantine merged pull request #9726:
URL: https://github.com/apache/kafka/pull/9726
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r568322390
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java
##########
@@ -265,6 +265,20 @@ public ConnectorInfo connectorInfo(String connector) {
);
}
+ protected Map<ConnectorTaskId, Map<String, String>> buildTasksConfig(String connector) {
+ final ClusterConfigState configState = configBackingStore.snapshot();
+
+ if (!configState.contains(connector))
+ return null;
Review comment:
I see the symmetry with `connectorInfo` above. But here we return a map, in which case it might be safer and nicer to return an empty map? Wdyt?
In terms of response the result will be the same because if the connector is found it will always have a map of maps.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java
##########
@@ -744,6 +744,29 @@ public Void call() throws Exception {
);
}
+ @Override
+ public void tasksConfig(String connName, final Callback<Map<ConnectorTaskId, Map<String, String>>> callback) {
+ log.trace("Submitting tasks config request {}", connName);
+
+ addRequest(
+ new Callable<Void>() {
Review comment:
I agree with your choice to preserve symmetry. Yet some features need to find their way to the codebase eventually. There's a PR in progress to apply some basic modernization and reduce boilerplate. Want to use a lambda and skip the need to synchronize merges with: https://github.com/apache/kafka/pull/9867 ?
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java
##########
@@ -434,4 +434,15 @@ public int hashCode() {
return Objects.hash(seq);
}
}
+
+ @Override
+ public void tasksConfig(String connName, Callback<Map<ConnectorTaskId, Map<String, String>>> callback) {
+ Map<ConnectorTaskId, Map<String, String>> tasksConfig = buildTasksConfig(connName);
+ if (tasksConfig == null) {
Review comment:
If you change above, don't forget to check with empty here.
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java
##########
@@ -519,6 +519,38 @@ public void testGetConnectorConfigConnectorNotFound() throws Throwable {
PowerMock.verifyAll();
}
+ @Test
+ public void testGetTasksConfig() throws Throwable {
Review comment:
Should we add one more test to test multiple tasks with multiple config properties?
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java
##########
@@ -519,6 +519,38 @@ public void testGetConnectorConfigConnectorNotFound() throws Throwable {
PowerMock.verifyAll();
}
+ @Test
+ public void testGetTasksConfig() throws Throwable {
+
Review comment:
nit: extra line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-773567218
Thanks @kkonstantine for the feedback. I've pushed an update
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-773648389
Just ran into a situation last night where this would have been super helpful. Thanks for adding this @mimaison!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r568322390
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java
##########
@@ -265,6 +265,20 @@ public ConnectorInfo connectorInfo(String connector) {
);
}
+ protected Map<ConnectorTaskId, Map<String, String>> buildTasksConfig(String connector) {
+ final ClusterConfigState configState = configBackingStore.snapshot();
+
+ if (!configState.contains(connector))
+ return null;
Review comment:
I see the symmetry with `connectorInfo` above. But here we return a map, in which case it might be safer and nicer to return an empty map? Wdyt?
In terms of response the result will be the same because if the connector is found it will always have a map of maps.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java
##########
@@ -744,6 +744,29 @@ public Void call() throws Exception {
);
}
+ @Override
+ public void tasksConfig(String connName, final Callback<Map<ConnectorTaskId, Map<String, String>>> callback) {
+ log.trace("Submitting tasks config request {}", connName);
+
+ addRequest(
+ new Callable<Void>() {
Review comment:
I agree with your choice to preserve symmetry. Yet some features need to find their way to the codebase eventually. There's a PR in progress to apply some basic modernization and reduce boilerplate. Want to use a lambda and skip the need to synchronize merges with: https://github.com/apache/kafka/pull/9867 ?
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java
##########
@@ -434,4 +434,15 @@ public int hashCode() {
return Objects.hash(seq);
}
}
+
+ @Override
+ public void tasksConfig(String connName, Callback<Map<ConnectorTaskId, Map<String, String>>> callback) {
+ Map<ConnectorTaskId, Map<String, String>> tasksConfig = buildTasksConfig(connName);
+ if (tasksConfig == null) {
Review comment:
If you change above, don't forget to check with empty here.
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java
##########
@@ -519,6 +519,38 @@ public void testGetConnectorConfigConnectorNotFound() throws Throwable {
PowerMock.verifyAll();
}
+ @Test
+ public void testGetTasksConfig() throws Throwable {
Review comment:
Should we add one more test to test multiple tasks with multiple config properties?
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java
##########
@@ -519,6 +519,38 @@ public void testGetConnectorConfigConnectorNotFound() throws Throwable {
PowerMock.verifyAll();
}
+ @Test
+ public void testGetTasksConfig() throws Throwable {
+
Review comment:
nit: extra line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-773648389
Just ran into a situation last night where this would have been super helpful. Thanks for adding this @mimaison!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#discussion_r570545828
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##########
@@ -188,6 +188,16 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+ @GET
+ @Path("/{connector}/tasks-config")
+ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(final @PathParam("connector") String connector,
+ final @Context HttpHeaders headers,
Review comment:
You mean something like this?
```java
@GET
@Path("/{connector}/tasks-config")
public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(
final @PathParam("connector") String connector,
final @Context HttpHeaders headers,
final @QueryParam("forward") Boolean forward) throws Throwable {
FutureCallback<Map<ConnectorTaskId, Map<String, String>>> cb = new FutureCallback<>();
...
````
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9726:
URL: https://github.com/apache/kafka/pull/9726#issuecomment-773567218
Thanks @kkonstantine for the feedback. I've pushed an update
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org