[GitHub] [kafka] mumrah opened a new pull request, #12403: KAFKA-13166 Fix missing ControllerApis error handling

[GitBox <gi...@apache.org>]

mumrah opened a new pull request, #12403:
URL: https://github.com/apache/kafka/pull/12403

   In ControllerApis, we are making use of the CompletableFuture-s returned by QuorumController to add callbacks via `whenComplete`. Since these chained methods on CompletableFuture return a _new_ CompletableFuture instance, the ControllerPurgatory does not capture any of downstream exceptions when completing the original instance. The only way to see these errors is to call `get` on the chained future, or to add additional completion stages to handle the error explicitly. However, we do not want to call `get` since that would block the request threads.
   
   This patch changes the signature of the request handlers in ControllerApis to all return CompletableFuture. It also converts all of the `CompletableFuture#whenComplete` callbacks to `CompletableFuture#handle`. This allows us to directly handle exceptions thrown by the controller in the individual request handlers. A single `whenComplete` error handler is added in `ControllerApis#handle` which will ensure we log exceptions thrown by the response builders (or other logic in the chained `handle` calls). 
   
   The resulting future chain now looks like:
   
   ```scala
   controller.doStuff()
       .handle { (result, exception) => 
         // Transform result or exception into a response 
       }
       .whenComplete { (result, exception) => 
         // Handle errors thrown in "handle"
       }
   ```
   
   The future is still completed by ControllerPurgatory, and that is when the completion stages are run. We do not call `get` in ControllerApis to avoid blocking the request threads.
   
   There is some opportunity for cleanup and consolidation of error handling in ControllerApis, but I'd rather make a separate PR for 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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] mumrah commented on pull request #12403: KAFKA-13166 Fix missing ControllerApis error handling

[GitBox <gi...@apache.org>]

mumrah commented on PR #12403:
URL: https://github.com/apache/kafka/pull/12403#issuecomment-1195746529

   Hey @cmccabe, thanks for the review! I agree it's a bit weird to return the completed feature here from the authorizer calls. I tried a few different approaches, and having all the handlers return the same type seemed to be a simple way to fix the error handling. The main issue I'm solving here is adding the final `whenComplete` to handle errors in completion handlers (which used to be `whenComplete`, but are now `handle` calls). 
   


-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] cmccabe commented on pull request #12403: KAFKA-13166 Fix missing ControllerApis error handling

[GitBox <gi...@apache.org>]

cmccabe commented on PR #12403:
URL: https://github.com/apache/kafka/pull/12403#issuecomment-1188236978

   So, we definitely need to get rid of the calls to *.get* and other cases of needless blocking. There are definitely some cases where we aren't properly handling errors as well (this is related I guess) in how we're chaining futures.
   
   The problem with returning CompletableFuture from everything is that there are things we do in the request handler threads that are blocking, such as anything the Authorizer does. I think it's kind of awkward to have half of the stuff blocking, and half non-blocking. I also see cases in this PR where we're returning a completed future, but the response hasn't been sent yet.
   
   Overall I'd prefer not to return futures from everything unless we can push this down to functions like `sendResponseMaybeThrottle`, which are currently callback-based rather than future-based. And the authorizer, of course, which is just blocking. I think mixing the two (three?) styles will lead to weird issues so we probably should just remove the calls to get and the error handling for now...


-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] cmccabe commented on pull request #12403: KAFKA-13166 Fix missing ControllerApis error handling

[GitBox <gi...@apache.org>]

cmccabe commented on PR #12403:
URL: https://github.com/apache/kafka/pull/12403#issuecomment-1195865697

   I looked into this more, and I think this approach could be a good start. But let's keep an eye out for if we can simplify this in the future. LGTM


-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] mumrah merged pull request #12403: KAFKA-13166 Fix missing ControllerApis error handling

[GitBox <gi...@apache.org>]

mumrah merged PR #12403:
URL: https://github.com/apache/kafka/pull/12403


-- 
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-unsubscribe@kafka.apache.org

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