[#4035] fix invocation context loss issue#4057
[#4035] fix invocation context loss issue#4057yanghao605 wants to merge 1 commit intoapache:masterfrom
Conversation
| public synchronized Span createSpan(Invocation invocation) { | ||
| return handler.handleSend(requestWrapper.invocation(invocation)); | ||
| } |
There was a problem hiding this comment.
This fix seems not correct. HttpClientResponseWrapper/HttpClientRequestWrapper should be invocation scope, not bean scope. Synchronization can not fix this problem .
There was a problem hiding this comment.
Based on my understanding of the brave's source code, I think only this part can set context to invocation.
When the handler. handleSend() method is called, it will insert the context related to trace into the invocation.
And after I added synchronized and tested again, there was no further loss of context
There was a problem hiding this comment.
Synchronization maybe very pool performance. Event your notice is correct, the modification is not good.
And maybe create a new wrapper instance is faster than synchronization.
2 participants
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles, where you replaceSCB-XXXwith the appropriate JIRA issue.mvn clean install -Pitto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.