Make some internal APIs more open

[timothybasanov]

Not a Contribution.

Currently a lot of internal APIs are well, internal. It should be possible to implement a different Timeout or Retry policy from an application package. This may require to make some package local methods and classes to be public.

[timothybasanov]

Not a contribution.

One example, it should be possible to implement Support staggered retries #291 or Considering allowing a Future result to be completed immediately when a Timeout occurs #231 using open APIs.

[jhalterman]

Sure, the goal is to make protected or public anything that needs to be to extend Failsafe while guarding the rest. Are there specific methods/fields/classes you'd like to see by made protected or public?

[timothybasanov]

Not a contribution.

Yep, all the methods and classes currently referenced from a timeout or retry policies and executions (e.g. an abstract execution's methods, a timeout execution itself). A test for this would be a custom timeout or retry policy in a different package that adjust existing ones a bit.

[jhalterman]

@timothybasanov I just pushed a big refactoring to the master branch to support a more proper SPI, where the various internals are exposed and creating custom policies should be more possible now. Have a look and please let me know what you think. I'd like to release this for 2.5.0 soon.

Most of the interesting things for you are now in the spi package. This is where PolicyExecutor now lives, which uses new ExecutionInternal, SyncExecutionInternal, and AsyncExecutionInternal interfaces for internal common, internal sync, and internal async execution behaviors respectively. Let me know how this looks and if I'm missing anything.

[jhalterman]

This issue auto-closed since I just merged the related changes to master. @timothybasanov let me know what you think if you can soon. Would be good before I make a 2.5 release.

[timothybasanov]

Not a Contribution

As an exercise I've tried to create a few custom policies. A few things that spring to my attention:

• Timeout only has a private constructor, so I can't extend it
• Replacing timeout's Duration with a lambda is hard. I have to pass null to a constructor as a workaround
• I can't create custom logic by subclassing TimeoutExecutor, it's package-private
• It's hard to make a RetryPolicy's duration mutable. I have to override all methods and guard them with locks to make it thread safe
• I can't replace jitter generation logic with a different one as RetryPolicyExecutor#adjustForJitter is private
• CircuitBreaker uses state.get() instead of getState(), and currentExecutions are private. This makes it impossible to have a custom recordSuccess()
• CircuitBreaker does not allow to replace onClose with a list. If I override close() I can't call a private transitionTo()
• Can't replace TimeoutExeededException with a subclass (with additional details e.g. duration). Most natural place is to replace PolicyExecutor#execution with a custom one, but AbstractExecution has a package-local constructor, on top of that it's not clear how can I wrap/intercept a call to Timeout#toExecutor which is done from places like ExecutionImpl which are also package-private

This is still a significant improvement over 2.4. I can now copy-paste the whole Timeout and its TimeoutExecutor to adjust the way I need it.

[jhalterman]

Thanks for the responses Timothy...

Timeout only has a private constructor, so I can't extend it

So you'd prefer a protected constructor? Are you using Timeout with the existing TImeoutExecutor or with your own? For 3.0 I'm actually planning to have private constructors on all policies and to let them be built via a Builder, FWIW, as part of #47.

Replacing timeout's Duration with a lambda is hard. I have to pass null to a constructor as a workaround

You mean something like Consumer<Duration> timeout, or something else? Maybe you can describe the use case?

I can't create custom logic by subclassing TimeoutExecutor, it's package-private

Hmm - maybe the PolicyExecutors can move to the internal package and as you say, become public.

It's hard to make a RetryPolicy's duration mutable

I assume you mean the lack of thread safety (#47)? I am working on that for 3.0, but the plan is to actually make policies immutable, except for adding event listeners, once they're built. Of course we could make it easy to create a new copy. Would that work for you? Maybe you can talk more about your use case.

I can't replace jitter generation logic with a different one

I can make those methods protected. I'm curious though what kind of jitter changes you're making. Perhaps it would be better to support some kind of jitter function config rather than having to override some methods in the PolicyExecutor?

CircuitBreaker uses state.get() instead of getState()

I'll make state and currentExecutions protected.

CircuitBreaker does not allow to replace onClose with a list. If I override close() I can't call a private transitionTo()

transitionTo could be made protected, but couldn't you still call super.close() to cause the transition? I'm not sure what you mean re: onClose and a list.

Can't replace TimeoutExeededException with a subclass

This might be better as a top ievel feature - same for CircuitBreakers. Does #225 sound right for that?

[timothybasanov]

Not a Contribution.

Timeout only has a private constructor, so I can't extend it So you'd prefer a protected constructor? Are you using Timeout with the existing TImeoutExecutor or with your own? For 3.0 I'm actually planning to have private constructors on all policies and to let them be built via a Builder, FWIW, as part of #47.

Yes, protected constructors for all policies would be appreciated. I often want to tweak only one aspect of the policy by extending it (e.g. getTimeout() using a lambda). If it's fully locked up, I'll have to create a copy-and-paste of the whole class for even minor modifications.

You mean something like Consumer<Duration> timeout, or something else? Maybe you can describe the use case? ... I'm curious though what kind of jitter changes you're making.

Yep, ideally a Function<SomeContext, Duration> for a timeout to have different timeouts for different retries, and a delay between retries to allow to use a pre-existing custom jitter function. For all my policies I have to be able to change all their parameters at runtime, so I prefer to have lambdas everywhere. Usually I add additional methods/constructors in a child class with lambdas instead of values and try to integrate them by auto-calling original setters when a lambda value changes.

Of course we could make it easy to create a new copy. Would that work for you?

Yep, if I can freely convert from policy to its builder and back, it would solve some of my issues. I would be able to store a "master copy" of a policy, and create "adjusted" ones when necessary for special cases. As an example different methods called on the same RPC service have very similar, but still slightly different configuration.

transitionTo could be made protected, but couldn't you still call super.close() to cause the transition? I'm not sure what you mean re: onClose and a list. I want to replace an onClose lambda with a list of lambdas to be able to add as many listeners as I want. The easiest way is to add a list of listeners as a field in a class and work with it instead of the original onClose. This requires a custom implementation of onClose.

Can't replace TimeoutExeededException with a subclass This might be better as a top ievel feature - same for CircuitBreakers. Does #225 sound right for that? Yep, you're right, it could be done with a Fallback policy without any changes to API.

These were only a few random examples. In general, I would recommend against locking up a framework with package locals or privates unless there is absolute certainty that end user would not want to reuse or adjust this functionality.

[whiskeysierra]

In general, I would recommend against locking up a framework with package locals or privates unless

I'd argue the opposite. Exposing internals and even allowing inheritance makes it extremely hard to maintain a library in the long run. I'd avoid inheritance like the plague and go with composition instead: https://whiskeysierra.github.io/knowledge/composition/

[Tembrel]

Amplifying what @whiskeysierra wrote: Making classes extensible is extremely hard to get right, and it's often at odds with the goal of a builder-based API. It can add maintenance costs that dominate the original re-use benefits.

I don't have a good answer to @timothybasanov — I understand the desire to be able to take advantage of all that working code that is currently locked up, but I'd hate to see the already severe maintenance burden increase further in an attempt to meet that desire.

[jhalterman]

@timothybasanov I just merged a PR into master which introduces a builder API in preparation for 3.0. I'm interested to get your revised take on how the new API works for you, and this issue.

[jhalterman]

3.0 has been released, which includes the SPI package. I'm still interested to hear how this works for you @timothybasanov in case there are more improvements needed.