Detaching request id from SqlRequest #81
Conversation
src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java
Show resolved
Hide resolved
| /** | ||
| * The key of the request id in the context map | ||
| */ | ||
| public static final String REQUEST_ID_KEY = "request_id"; |
There was a problem hiding this comment.
private may be better? Since this is more like your internal detail rather than a public constant.
There was a problem hiding this comment.
Agree, no reason to keep it public. I was trying to do something fancier to basically check if the elastic's thread context has the id and get it from there, that's why made this public, but that would be a different change anyway (if we ever make it), changing this to private.
| @After | ||
| public void cleanUpContext() { | ||
|
|
||
| ThreadContext.clearMap(); |
There was a problem hiding this comment.
When this or cleanup of context happen in our plugin? Or we just generate an ID for each thread and keep using it forever?
There was a problem hiding this comment.
this never happens, it's just a way to make the test cases independent.
There was a problem hiding this comment.
also, we should generate only once for the main (accepting the request thread), and pass it over to other threads created from within that one. As long as the threads are working on the same request, they should share the same id.
There was a problem hiding this comment.
this never happens, it's just a way to make the test cases independent.
Does this mean for all the following request served by the thread, we keep using same ID?
There was a problem hiding this comment.
also, we should generate only once for the main (accepting the request thread), and pass it over to other threads created from within that one. As long as the threads are working on the same request, they should share the same id.
Need more context how this works. So you're saying we generate an ID for each transport thread only once. Then I saw you "preserve" the context when you pass it to worker thread. So for all the logging on the path (for a transport thread), same ID would be in use always between requests?
There was a problem hiding this comment.
OK, I see your point now. You're right, I need to clean up the request id after the processing is done or overwrite it when the processing starts. It should be per request, not per thread, and with current code it won't work for re-used threads.
src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java
Show resolved
Hide resolved
*Issue:* opendistro-for-elasticsearch#34 *Description of changes:* Removed id field from SqlRequest class. Using logging library's ThreadContext to store the request id as soon as we get it, and passing it around to make it available in case new threads are started from the main one. Provided a utility class to make the access to the ThreadContext simpler. The current layout pattern of elasticsearch log messages is [%d{ISO8601}][%-5p][%-25c{1.}] [%node_name]%marker %m%n, if we pre-pend [%X{request_id}] all the messages will automatically be pre-pended with the request id. However, the plugin code does not manage the log configuration, and I didn't want to modify it at run-time. *How tested:* Made some requests on local cluster, made sure request id is getting properly populated. Tested for tasks running on worker threads as well.
Issue #, if available: #34
Description of changes: Removed
idfield fromSqlRequestclass. Using logging library's ThreadContext to store the request id as soon as we get it, and passing it around to make it available in case new threads are started from the main one. Provided a utility class to make the access to the ThreadContext simpler.The current layout pattern of elasticsearch log messages is
[%d{ISO8601}][%-5p][%-25c{1.}] [%node_name]%marker %m%n, if we pre-pend[%X{request_id}]all the messages will automatically be pre-pended with the request id. However, the plugin code does not manage the log configuration, and I didn't want to modify it at run-time.How tested: Made some requests on local cluster, made sure request id is getting properly populated. Tested for tasks running on worker threads as well.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.