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

Configuration do not cache sender instance #685

Merged
merged 1 commit into from
Feb 17, 2020
Merged

Configuration do not cache sender instance #685

merged 1 commit into from
Feb 17, 2020

Conversation

ehsavoie
Copy link
Contributor

@ehsavoie ehsavoie commented Feb 12, 2020 •
edited by pavolloffay
Loading

Resolves #684

  • Adding a closed attribute on sender
  • Sender configuration is resolving again is sender is closed.

Issue: #684

@codecov
Copy link

codecov bot commented Feb 12, 2020 •
edited
Loading

Codecov Report

Merging #685 into master will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #685      +/-   ##
============================================
+ Coverage     89.46%   89.60%   +0.13%     
- Complexity      567      568       +1     
============================================
  Files            69       69              
  Lines          2089     2088       -1     
  Branches        267      266       -1     
============================================
+ Hits           1869     1871       +2     
+ Misses          137      135       -2     
+ Partials         83       82       -1
Impacted Files Coverage Δ Complexity Δ
...n/java/io/jaegertracing/internal/JaegerTracer.java 89.07% <0.00%> (+1.06%) 27.00% <0.00%> (+1.00%) ⬇️

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 74adce4...cddf198. Read the comment docs.

@ehsavoie ehsavoie mentioned this pull request Feb 12, 2020
Closed
@pavolloffay
Copy link
Member

pavolloffay commented Feb 12, 2020 •
edited
Loading

There are couple of inconsistencies in the Configuration class. For instance some objects are cached (tracer, sender) and some not.

Also weird is that Configuration.getTracerBuilder() returns a new builder instance even though a tracer has been initialized and it is cached in the class.

Instead of adding a close to Sender I am wondering whether a better solution would be to do not cache configuration objects in getTracerBuilder and always resolve new instances.This also helps if configuration changes.

@ehsavoie
Copy link
Contributor Author

@pavolloffay I agree, a simpler solution is to remove those cached instances. Just did that :)

@pavolloffay pavolloffay mentioned this pull request Feb 13, 2020
Merged
@pavolloffay pavolloffay changed the title Recreating sender when a sender has been closed. Recreating sender when a sender has been closed Feb 14, 2020
@ehsavoie
Removing cached sender from Configuration.
cddf198 
This allows the new instance to be up-to-date with of configuration changes.

Resolves jaegertracing#684
     * Adding a closed attribute on sender
     * Sender configuration is resolving again is sender is closed.
Signed-off-by: Emmanuel Hugonnet <[email protected]>
@pavolloffay pavolloffay changed the title Recreating sender when a sender has been closed Configuration do not cache sender instance Feb 17, 2020
@yurishkuro yurishkuro merged commit 5e81d89 into jaegertracing:master Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configuration shouldn't keep a reference to a closed sender
3 participants
@ehsavoie @pavolloffay @yurishkuro