[GitHub] [kafka] kkonstantine commented on a change in pull request #9726: KAFKA-10833: Expose task configurations in Connect REST API

[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