gianm commented on code in PR #20005:
URL: https://github.com/apache/druid/pull/20005#discussion_r3785818176
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RemoteTaskActionClient.java:
##########
@@ -63,19 +63,17 @@ public <RetType> RetType submit(TaskAction<RetType>
taskAction) throws IOExcepti
// We're using a ServiceClient directly here instead of OverlordClient,
because OverlordClient does
// not have access to the TaskAction class. (OverlordClient is in the
druid-server package, and TaskAction
// is in the druid-indexing-service package.)
- final Map<String, Object> response = jsonMapper.readValue(
+ return jsonMapper.<TaskActionResponse<RetType>>readValue(
client.request(
new RequestBuilder(HttpMethod.POST, "/druid/indexer/v1/action")
.jsonContent(jsonMapper, new TaskActionHolder(task,
taskAction)),
- new BytesFullResponseHandler()
- ).getContent(),
- JacksonUtils.TYPE_REFERENCE_MAP_STRING_OBJECT
- );
-
- return jsonMapper.convertValue(
- response.get("result"),
- taskAction.getReturnTypeReference()
- );
+ new InputStreamResponseHandler()
Review Comment:
IMO this is OK, for two reasons.
1) Most failures are going to happen before the response body starts being
sent. Generally once that starts, most failures we see would be timeouts or
OOMs due to too much data being sent, which isn't generally going to be fixed
by a retry anyway. There is precedent for not retrying these failures,
especially in `CoordinatorClientImpl` and `OverlordClientImpl`.
2) The alternative is the prior state (not releasing bytes to the JSON
parser until they have been fully buffered). In theory it would be possible to
incrementally parse the bytes inside an `HttpResponseHandler` that returns an
unfinished response, but I don't see a clean way to do this with Jackson. The
`JsonParser` methods that drive parsing all expect to block while they fetch
all their input, which would necessitate another thread, which I'd like to
avoid. `NonBlockingJsonParser` is a thing that does exist in Jackson, but it
doesn't seem to work with `jackson-databind`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
