search

opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Make GlobalTracer uses more threadsafe #269

Closed sjoerdtalsma closed 6 years ago

sjoerdtalsma commented 6 years ago

As discussed in #186 replace void register(Tracer) with a more atomic boolean registerIfAbsent(Tracer) method.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.7%) to 74.112% when pulling 11b016dbb6d9f2c73291d633d7753b221c9e7a70 on sjoerdtalsma:globaltracer-concurrency into e385f73f6967583181c89e3d119ea717a2180ec3 on opentracing:master.

yurishkuro commented 6 years ago

Callable sgtm

sjoerdtalsma commented 6 years ago

@pavolloffay Have your comments been addressed?

sjoerdtalsma commented 6 years ago

@pavolloffay @yurishkuro Are there any remaining concerns regarding this PR?

Otherwise it may be a good idea to merge this soon, to allow people to adapt to the deprecation as soon as possible.

yurishkuro commented 6 years ago

lgtm, 🚢

sjoerdtalsma commented 6 years ago

The last activity on this PR is almost a month ago with three approvals given already. @pavolloffay do you have any remaining concerns regarding this PR?

Otherwise I suggest we merge this soon, so people can get used to the deprecated method and its replacement.

pavolloffay commented 6 years ago

@sjoerdtalsma sorry for long delay, thanks for the PR!

pavolloffay commented 6 years ago

I think we could/should still keep GlobalTracer.register(tracer) or registerIfAbsent(Tracer) for simpler use cases. Integrations in runtimes usually controls tracer initialization.

pavolloffay commented 6 years ago

@sjoerdtalsma do you want to tackle it in this PR? If not let's merge this and do it separately.

pavolloffay commented 6 years ago

I am also a bit concerned about callable. Maybe a separate interface would be better. Callable somehow indicates use of executors which is not the case here.

sjoerdtalsma commented 6 years ago

@pavolloffay

I think we could/should still keep GlobalTracer.register(tracer) or registerIfAbsent(Tracer) for simpler use cases. Integrations in runtimes usually controls tracer initialization.

I'm not sure I understand what you mean here. registerIfAbsent is a replacement of register. Do you know of a use-case for register that isn't possible with registerIfAbsent? It is entirely possible that I misunderstood your comment.

I am also a bit concerned about callable. Maybe a separate interface would be better. Callable somehow indicates use of executors which is not the case here.

My ideal would be to use a Supplier<Tracer> but that doesn't exist in our java version yet.
I actually started with a separate TracerSupplier interface. Then you asked whether it was necessary. Discussion and agreement amounted to just using Callable and wrap the checked exceptions as the most simple solution and not worthy of a separate interface just for this purpose.

I'm not against re-introducing the separate interface again, but maybe we're running in circles on this one?

sjoerdtalsma commented 6 years ago

Maybe someone should just merge this in and if there are many questions re: Callable, replace it by TracerSupplier at that time?

pavolloffay commented 6 years ago

@sjoerdtalsma I mean we should also keep simple registerIfAbsent(Tracer tracer) without callable. Usually, runtimes control tracer initialization so there is no need for callable!

pavolloffay commented 6 years ago

This can go in registerIfAbsent(Tracer tracer) can be added in a separate PR

sjoerdtalsma commented 6 years ago

@pavolloffay

This can go in registerIfAbsent(Tracer tracer) can be added in a separate PR

It may be good to discuss the Tracer variant in a separate PR indeed, re: @yurishkuro's comment here: https://github.com/opentracing/opentracing-java/pull/269#discussion_r181815569

Thanks everyone for their review, could someone with merge rights merge this then? :wink:

pavolloffay commented 6 years ago

@sjoerdtalsma think does not work for me, I will merge this now