DRILL-8307: Ensure thread safety in the Druid plugin HTTP client #2650
Conversation
jnturton
added
bug
backport-to-stable
|
I see I missed a spot, converting to draft. |
jnturton
force-pushed
the
8307-druid-http-threads
branch
from
bbfb4e7 to
57be903
Compare
| } | ||
| try (Response response = restClient.post(queryUrl, query)) { | ||
| if (!response.isSuccessful()) { | ||
| throw UserException |
There was a problem hiding this comment.
Would it be possible to get the errorContext from the DruidBatchReader so that we get good error messages?
There was a problem hiding this comment.
I see that these objects are created in the DruidPlguin but maybe we could add a method addErrorContext or something like that and call it in the constructor of the Batch Reader.
There was a problem hiding this comment.
@cgivre thanks, I'll check that out. I'd have thought that any error information available in the batch reader would have to also be present here in the HTTP response. Even if that's true, there might be some more response body unpacking to do to expose it.
There was a problem hiding this comment.
@cgivre I wasn't aware of the CustomErrorContext, thanks for pointing it out. It looks to me like it's an EVF construct that isn't accessible to pre-EVF plugins like storage-druid. If I've got that right then will we only be able to start it using in this plugin's error messages when its conversion to EVF arrives?
There was a problem hiding this comment.
Ahh... you're right. In that case, would you please put some TODO comments in the rest clients and then we can update once the EVF conversion is done?
There was a problem hiding this comment.
I'm good with merging this once that's done. +1
jnturton
force-pushed
the
8307-druid-http-threads
branch
from
e523491 to
529b510
Compare
| HttpEntity entity = new StringEntity(body, DEFAULT_ENCODING); | ||
| httppost.setEntity(entity); | ||
| public Response post(String url, String body) throws IOException { | ||
| RequestBody postBody = RequestBody.create(body.getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Would it be possible to use the RequestBody.create method that takes a String instead of converting to a byte[]?
There was a problem hiding this comment.
Would it be possible to use the RequestBody.create method that takes a String instead of converting to a byte[]?
Ugh I missed that. @cgivre please may we incorporate @pjfanning's suggestion here in the EVF conversion PR.
| public Response get(String url) throws IOException { | ||
| Request get = new Request.Builder() | ||
| .url(url) | ||
| .addHeader("Content-Type", "application/json") |
There was a problem hiding this comment.
Why would a get request need a content-type? Content-Type on a request refers to the type of the request body but for GETs, this is empty. Maybe I've misunderstood the intent, but shouldn't this be an Accept header? An Accept header is used to tell the HTTP server that the client (us) would prefer to get the response with a particular content-type. See https://en.wikipedia.org/wiki/List_of_HTTP_header_fields which has a description of what the headers mean.
There was a problem hiding this comment.
@pjfanning: also a valid question, but remember that this PR was a thread safety fix intended for backporting to stable so I strove to disturb as little possible while fixing the bug. Since the plugin specified this content-type here before, and we have no bug report related to it doing so, it continues to do that after this PR.
Maybe when @cgivre sends in the EVF version of this plugin we can see if Druid is indeed happy for this content-type header to disappear. I'm almost sure you're right and that will be the case.
3 participants
DRILL-8307: Ensure thread safety in the Druid plugin HTTP client
Description
Replaces the Apache HttpClient in the Druid storage plugin with the thread-safe OkHttp3 client.
Documentation
N/A
Testing
Existing unit tests.