Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Senders fix, do not use static thrift factory #233

Merged

Conversation

pavolloffay
Copy link
Member

When I was using multiple tracers in one process HttpSender was sending corrupted data.

@codecov-io
Copy link

Codecov Report

Merging #233 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #233      +/-   ##
============================================
- Coverage     81.89%   81.87%   -0.02%     
+ Complexity      525      524       -1     
============================================
  Files            87       87              
  Lines          2005     2003       -2     
  Branches        236      236              
============================================
- Hits           1642     1640       -2     
  Misses          263      263              
  Partials        100      100
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/uber/jaeger/senders/ThriftSender.java 73.46% <ø> (ø) 10 <0> (ø) ⬇️
...c/main/java/com/uber/jaeger/senders/UdpSender.java 69.23% <100%> (-2.2%) 4 <1> (-1)
.../main/java/com/uber/jaeger/senders/HttpSender.java 90.32% <100%> (-0.31%) 7 <2> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 912bfdb...a58b860. Read the comment docs.

Copy link
Contributor

@vprithvi vprithvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good. Do you think it’s worth having a test case with concurrent tracers?

@pavolloffay
Copy link
Member Author

We would have to emulate deserialization on the server size.

Could we get it in and if necessary write test in separate PR? It's blocking my work spark-depenencies PR https://github.com/pavolloffay/spark-dependencies/pull/1.

@pavolloffay
Copy link
Member Author

Once this is merged. Could you please do micro release? (I don't have rights and @jpkrohling will be here on Wednesday)

@vprithvi
Copy link
Contributor

Yeah, we can write a test as a separate PR. I'll merge and release

@vprithvi vprithvi merged commit 272ca0e into jaegertracing:master Sep 15, 2017
@yurishkuro
Copy link
Member

@pavolloffay are you saying the factory is not thread safe?

@pavolloffay
Copy link
Member Author

I haven't digged very deep. I will write a test later and look more closely.

@vprithvi
Copy link
Contributor

The serializer isn't threadsafe, but looking at https://github.com/apache/thrift/blob/master/lib/java/src/org/apache/thrift/protocol/TBinaryProtocol.java#L85 it seems that we should be able to continue having the factory as static.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants