Do not make isRegistered() synchronized. #333
Conversation
This one does not really need to be synchronized. The field itself is volatile and that is enough.
| @@ -95,7 +95,7 @@ public static Tracer get() { | |||
| * | |||
| * @return Whether a tracer has been registered | |||
| */ | |||
| public static synchronized boolean isRegistered() { return isRegistered; } | |||
| public static boolean isRegistered() { return isRegistered; } | |||
There was a problem hiding this comment.
Agree, I approve of this, but would still love this condition to be deduced from the tracer itself instead of the artificial boolean.
I prefer a unique 'unregistered' instance (behaviourally identical to NoopTracer) that can be compared by reference:
return tracer != UNREGISTERED.
This makes the atomic registration also semantically 'just register' the tracer instead of also setting some boolean.
There was a problem hiding this comment.
In that case we would need to allow the NoopTracerFactory to have more than an instance (currently a singleton), so we can apply a check against the very initial 'unregistered' instance ;)
Perhaps as an improvement after 0.32 is released?
There was a problem hiding this comment.
what, if any, would be user-visible change in behavior if internal boolean is removed?
There was a problem hiding this comment.
Perhaps as an improvement after 0.32 is released?
Agree, I approve of this PR, my comment was intended for further improvement.
what, if any, would be user-visible change in behavior if internal boolean is removed?
I would hope none. Just more consistent internal bookkeeping (single variable instead of two to maintain).
There was a problem hiding this comment.
So we will look into this fine improvement (along probably a change into the no-op tracer factory) right after 0.32 ;)
* Deprecate the StringTag.set() overload taking a StringTag. (#262) * Implement Trace Identifiers. (#280) * Bump JaCoCo to use a Java 10 friendly version (#306) * Remove finish span on close (#301) * Deprecate finishSpanOnClose on activation. * Add ScopeManager.activeSpan() and Tracer.activateSpan(). * Clarify the API changes and deprecations. * Add an error reporting sample to opentracing-testbed. * Simple layer on top of ByteBuffer for BINARY format. (#276) * Add generic typed setTag/withTag (#311) * Allow injecting into maps of type Map<String,Object> (#310) * Add simple registerIfAbsent to global tracer (#289) * Split Inject and Extract Builtin interfaces (#316) * Deprecate ScopeManager.active() (#326) * Make Tracer extends Closable. (#329) * Do not make isRegistered() synchronized. (#333) * Deprecate AutoFinishScopeManager (#335)
This one does not really need to be synchronized.
The field itself is volatile and that is enough.
Probably being too pedantic on this, but it feels this is the correct thing to do ;)
@yurishkuro @sjoerdtalsma