Remove deprecated API - HttpSender and RemoteBaggMana

[pavolloffay]

Related to #414 (comment)

Signed-off-by: Pavol Loffay ploffay@redhat.com

[jpkrohling]

LGTM

[pavolloffay]

Note that now it's not possible to override this class

[jpkrohling]

Not good :)

[pavolloffay]

Note that now it's not possible to override this class

[jpkrohling]

That's not good, then.

[yurishkuro]

cc @black-adder

[black-adder]

oops, my bad, we shouldn't deprecate this

[pavolloffay]

We want to deprecate it and use builder instead. Does protected constructor work for you?

[black-adder]

that's fine

[pavolloffay]

thanks the private with the most parameters is changed from private to protected

protected RemoteBaggageRestrictionManager(
       String serviceName,
       BaggageRestrictionManagerProxy proxy,
       Metrics metrics,
       boolean denyBaggageOnInitializationFailure,
       int refreshIntervalMs,
       int initialDelayMs
   ) {

[codecov]

Codecov Report

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

@@             Coverage Diff              @@
##             master     #431      +/-   ##
============================================
+ Coverage     87.15%   87.32%   +0.16%     
+ Complexity      510      509       -1     
============================================
  Files            65       65              
  Lines          1978     1972       -6     
  Branches        258      258              
============================================
- Hits           1724     1722       -2     
+ Misses          164      160       -4     
  Partials         90       90

Impacted Files	Coverage Δ	Complexity Δ
...acing/baggage/RemoteBaggageRestrictionManager.java	100% <ø> (+6.55%)	10 <0> (ø)	⬇️
...main/java/io/jaegertracing/senders/HttpSender.java	92.45% <ø> (-0.28%)	5 <0> (-1)

---

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 71b1a50...2841c44. Read the comment docs.

[jpkrohling]

Let's clarify the need for subclassing HttpSender and the RemoteBaggageRestrictionManager.

[pavolloffay]

I am not sure if people are overriding it. We can make builder constructors provided is needed.

[jpkrohling]

I am not sure if people are overriding it.

It's more about the message we are sending: do we let subclass it, or should they never subclass it? If the HttpSender is part of the public API, then it should be extensible.

[pavolloffay]

If the HttpSender is part of the public API, then it should be extensible.

I am not quite sure about ^^. There are public APIs which are not extensible. Maybe our question is: is there a reason to extend it? Do people need it?

From the API design perspective, visibility should be restricted as much as possible. If people ask for it make it public

[jpkrohling]

There are public APIs which are not extensible

True, but then they marked as "final".

Maybe our question is: is there a reason to extend it? Do people need it?

That's indeed the question. There's only one method that could be overridden, so, one could just extend ThriftSender directly, just like the HttpSender does.

Dismissing review, to discuss whether HttpSender/RemoteBaggageRestrictionManager should be extensible