-
Notifications
You must be signed in to change notification settings - Fork 896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #850: Added request context across client and bookies. #1672
Conversation
…nt and bookies. - MDC context is passed to runnables, callables etc. - protocol extended, context is sent to bookie servers, restored theer and back on client with the response. Hopefully did not miss some nuance on the server side, largely rely on changes in ordered executors to do all the magic. - did microbenchmarking of the protocol changes (strings added to protobuf, MDC context preserved/restored). Looks ok. - added unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks great to me. this is a great addon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, a useful feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome !
+1
|
||
/** | ||
* Flag describing executor's expectation in regards of MDC. | ||
* All tasks submittedt through executor's submit/execute methods will automatically respect this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/submittedt/submitted
@@ -412,6 +415,7 @@ private OrderedExecutor createExecutor( | |||
.numThreads(numThreads) | |||
.name(nameFormat) | |||
.traceTaskExecution(serverCfg.getEnableTaskExecutionStats()) | |||
.preserveMdcForTaskExecution(serverCfg.getPreserveMdcForTaskExecution()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use local cached value instead of getting from conf again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this the way it is.
createExecutor() is called in the constructor so changing it to the local value creates expectations for the order of the value initialization.
Performance-wise it is a noop since it is only used in the constructor.
break; | ||
} | ||
} finally { | ||
MDC.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call it conditionally? Only if the conf is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but MDC is not set otherwise so clear (that checks if underlying map is not empty) vs checking for that flag are the same at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With minor comments; LGTM
…pache#169) Descriptions of the changes in this PR: - MDC context is passed to runnables, callables etc. - protocol extended, context is sent to bookie servers, restored there and back on client with the response. Hopefully did not miss some nuance on the server side, largely rely on changes in ordered executors to do all the magic. - did microbenchmarking of the protocol changes (strings added to protobuf, MDC context preserved/restored). Looks ok. - added unit tests. (bug W-5291641) (bug W-5291648) Troubleshooting of request-level failures/errors can be simplified if request id was passed through all the stages of the request, from threadpools on the client to bookies to the response back on the client. Log4j/Slf4j allows logging of MDC data so it makes sense to use this functionality for logging. - MDC context is passed to runnables, callables etc. - protocol extended, context is sent to bookie servers, restored there and back on client with the response. Hopefully did not miss some nuance on the server side, largely rely on changes in ordered executors to do all the magic. - did microbenchmarking of the protocol changes (strings added to protobuf, MDC context preserved/restored). Looks ok. - added unit tests. Master Issue: apache#850 Author: Andrey Yegorov <ayegorov@salesforce.com> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None>, Venkateswararao Jujjuri (JV) <None>, Sijie Guo <sijie@apache.org> This closes apache#1672 from dlg99/feature/correlation_id, closes apache#850 @Rev vjujjuri@ @Rev cguttapalem@
Descriptions of the changes in this PR:
Hopefully did not miss some nuance on the server side, largely rely on changes in ordered executors to do all the magic.
(@bug W-5291641@)
(@bug W-5291648@)
Motivation
Troubleshooting of request-level failures/errors can be simplified if request id was passed through all the stages of the request, from threadpools on the client to bookies to the response back on the client.
Log4j/Slf4j allows logging of MDC data so it makes sense to use this functionality for logging.
Changes
Hopefully did not miss some nuance on the server side, largely rely on changes in ordered executors to do all the magic.
Master Issue: #850