You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/30 22:22:49 UTC
[GitHub] [druid] gianm opened a new pull request #12011: Enhancements to IndexTaskClient.
gianm opened a new pull request #12011:
URL: https://github.com/apache/druid/pull/12011
1) Ability to use handlers other than StringFullResponseHandler. This
functionality is not used in production code yet, but is useful
because it will allow tasks to communicate with each other in
non-string-based formats and in streaming fashion. In the future,
we'll be able to use this to make task-to-task communication
more efficient.
2) Truncate server errors at 1KB, so long errors do not pollute logs.
3) Change error log level for retryable errors from WARN to INFO. (The
final error is still WARN.)
4) Harmonize log and exception messages to have a more consistent format.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm edited a comment on pull request #12011: Enhancements to IndexTaskClient.
Posted by GitBox <gi...@apache.org>.
gianm edited a comment on pull request #12011:
URL: https://github.com/apache/druid/pull/12011#issuecomment-983943224
The patch was getting nailed on test coverage for IndexTaskClient. I added a bunch of tests, and while doing so, noticed that the FullResponseHolder needlessly stored `status` separately from `response`. All call sites populated status with `response.getStatus()` anyway, whose main implementation in DefaultHttpResponse is a simple field access. So I changed the holder to use `response.getStatus()` itself and removed the `status` field.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12011: Enhancements to IndexTaskClient.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #12011:
URL: https://github.com/apache/druid/pull/12011#issuecomment-983943224
The patch was getting nailed on test coverage for IndexTaskClient. I added a bunch of tests, and while doing so, noticed that the FullResponseHolder needlessly stored `status` separately from `response`. All call sites populated status with `response.getStatus()` anyway, whose main implementation is DefaultHttpResponse, which stores `status` in a field itself. So I changed the holder to use `response.getStatus()` itself and removed the `status` field.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on a change in pull request #12011: Enhancements to IndexTaskClient.
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12011:
URL: https://github.com/apache/druid/pull/12011#discussion_r767316253
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/IndexTaskClient.java
##########
@@ -262,6 +276,7 @@ protected StringFullResponseHolder submitSmileRequest(
encodedPathSuffix,
encodedQueryString,
content,
+ new StringFullResponseHandler(StandardCharsets.UTF_8),
Review comment:
with this change, I believe the following code paths are no longer reachable, since the response would always be a success
- https://github.com/apache/druid/blob/2df0f52cfad28c3be2f43fb57b9ca3415c6f3eea/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTaskClient.java#L66-L71
- https://github.com/apache/druid/blob/2df0f52cfad28c3be2f43fb57b9ca3415c6f3eea/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTaskClient.java#L101-L106
- https://github.com/apache/druid/blob/2df0f52cfad28c3be2f43fb57b9ca3415c6f3eea/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTaskClient.java#L129-L133
The calls to `IndexTaskClient.isSuccess(StringFullResponseHolder)` from `SeekableStreamIndexTaskClient` would also no longer be necessary.
As a result I believe we can remove the `IndexTaskClient.isSucess()` method entirely
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #12011: Enhancements to IndexTaskClient.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #12011:
URL: https://github.com/apache/druid/pull/12011
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org