feat(client): support push query termination in Java client #5371
Conversation
| recordParser.exceptionHandler(responseHandler::handleException); | ||
| } | ||
|
|
||
| private static void handleSuccessfulCloseQueryResponse(final CompletableFuture<Void> cf) { |
There was a problem hiding this comment.
These methods and the one above seem like overkill - I'd probably just put them in the lambda.
There was a problem hiding this comment.
As above, I've refactored this. Hopefully your concerns have been addressed indirectly.
| @@ -158,15 +188,10 @@ private HttpClientRequest configureBasicAuth(final HttpClientRequest request) { | |||
| private static <T> void handleResponse( | |||
| final HttpClientResponse response, | |||
| final CompletableFuture<T> cf, | |||
There was a problem hiding this comment.
Seems odd to pass in both a cf and a Handler to signal back both exceptionally and successful results. Can't the cf do both?
There was a problem hiding this comment.
Refactored this to be less convoluted.
| "/query-stream", | ||
| response -> handleResponse(response, cf, responseHandlerSupplier)) | ||
| path, | ||
| response -> handleResponse(response, cf, successfulResponseHandler)) |
There was a problem hiding this comment.
I don't think you need to pass in successfulResponseHandler here, you can just call cf.thenAccept(this::successfulResponseHandler) (or similar) in this method.
There was a problem hiding this comment.
Not quite. The CompletableFuture will succeed even if the response status is not 200. The successful response handler is only meant to be called if the response status is 200, else the error handling logic should be called. I realize the way this is currently set up is a bit convoluted, though, so I've refactored it to be more readable.
There was a problem hiding this comment.
Thanks for the review, @purplefox ! I've added a slight refactor to (hopefully) make this more readable. LMK if I haven't addressed your concerns and I'll open a follow-up.
| "/query-stream", | ||
| response -> handleResponse(response, cf, responseHandlerSupplier)) | ||
| path, | ||
| response -> handleResponse(response, cf, successfulResponseHandler)) |
There was a problem hiding this comment.
Not quite. The CompletableFuture will succeed even if the response status is not 200. The successful response handler is only meant to be called if the response status is 200, else the error handling logic should be called. I realize the way this is currently set up is a bit convoluted, though, so I've refactored it to be more readable.
| @@ -158,15 +188,10 @@ private HttpClientRequest configureBasicAuth(final HttpClientRequest request) { | |||
| private static <T> void handleResponse( | |||
| final HttpClientResponse response, | |||
| final CompletableFuture<T> cf, | |||
There was a problem hiding this comment.
Refactored this to be less convoluted.
| recordParser.exceptionHandler(responseHandler::handleException); | ||
| } | ||
|
|
||
| private static void handleSuccessfulCloseQueryResponse(final CompletableFuture<Void> cf) { |
There was a problem hiding this comment.
As above, I've refactored this. Hopefully your concerns have been addressed indirectly.
Description
^ via the
/close-queryendpoint. This PR also adds a test to check thatStreamedQueryResultobjects are properly completed when the underlying query is terminated.Testing done
Unit tests.
Reviewer checklist