[GitHub] [kafka] rhauch commented on a change in pull request #11132: KAFKA-13139: Empty response after requesting to restart a connector without the tasks results in NPE

2021-07-27 Thread GitBox 


rhauch commented on a change in pull request #11132:
URL: https://github.com/apache/kafka/pull/11132#discussion_r677530176



##
File path: 
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##
@@ -269,7 +269,7 @@ public Response restartConnector(final 
@PathParam("connector") String connector,
 FutureCallback cb = new FutureCallback<>();
 herder.restartConnector(connector, cb);
 completeOrForwardRequest(cb, forwardingPath, "POST", headers, 
null, forward);
-return Response.ok().build();
+return Response.noContent().build();

Review comment:
   I've corrected the KIP and sent an email describing this minor 
correction to the vote thread for the KIP.
   
   I've also added [a comment on 
KAFKA-13139](https://issues.apache.org/jira/browse/KAFKA-13139?focusedCommentId=17388116=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17388116)
 that describes the root cause and the KIP correction.
   
   Thanks, @kkonstantine and @kpatelatwork.




-- 
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.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] rhauch commented on a change in pull request #11132: KAFKA-13139: Empty response after requesting to restart a connector without the tasks results in NPE

2021-07-26 Thread GitBox 


rhauch commented on a change in pull request #11132:
URL: https://github.com/apache/kafka/pull/11132#discussion_r677104692



##
File path: 
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##
@@ -269,7 +269,7 @@ public Response restartConnector(final 
@PathParam("connector") String connector,
 FutureCallback cb = new FutureCallback<>();
 herder.restartConnector(connector, cb);
 completeOrForwardRequest(cb, forwardingPath, "POST", headers, 
null, forward);
-return Response.ok().build();
+return Response.noContent().build();

Review comment:
   Awesome. Thanks, folks!




-- 
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.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] rhauch commented on a change in pull request #11132: KAFKA-13139: Empty response after requesting to restart a connector without the tasks results in NPE

2021-07-26 Thread GitBox 


rhauch commented on a change in pull request #11132:
URL: https://github.com/apache/kafka/pull/11132#discussion_r676987061



##
File path: 
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java
##
@@ -269,7 +269,7 @@ public Response restartConnector(final 
@PathParam("connector") String connector,
 FutureCallback cb = new FutureCallback<>();
 herder.restartConnector(connector, cb);
 completeOrForwardRequest(cb, forwardingPath, "POST", headers, 
null, forward);
-return Response.ok().build();
+return Response.noContent().build();

Review comment:
   This returns `204 No Content`, right? But doesn't the old code (prior to 
this feature) return `200 OK` with no content? 
   
   
[KIP-745](https://cwiki.apache.org/confluence/display/KAFKA/KIP-745%3A+Connect+API+to+restart+connector+and+tasks)
 mentions using `202 Accepted` for the new behavior, but keeps the `200 OK` 
behavior when `includeTasks=false` and `onlyFailed=false`. 
   
   Are you suggesting that we can't return `200 OK` here if there is no 
response body and should therefore update the KIP, or can this line be left as 
is and we change the `RestClient` to properly handle a null content? The issue 
for this test failure ([KAFKA-111]()) mentions [this 
code](https://github.com/apache/kafka/blob/d89ea74918ce0a1f2309a09473c9500c52af3b10/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java#L134-L136)
 in the `RestClient`, which handles all 200 to <300 status codes the same way, 
so could we just improve that?




-- 
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.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org