Introduce policy builders

[jhalterman]

This PR refactors Failsafe to provide builders for creating policies. Along the way it makes several changes to how thing are organized:

• Policies are now defined as interfaces, with a separate impl in the .internal package
• A Builder class exists for each policy, to define optional policy configuration
  • A builder can be created by calling .builder() on a policy class
  • By convention, policies that have required arguments like Fallback and Timeout have builder methods that accept the required parameter, ex: Timeout.builder(Duration), with optional parameters being defined in the Builder class itself.
• A Config class exists for each policy, to read policy configuration
  • The reason for creating a separate Config class for each policy is that, for the most part, only the PolicyExecutor implementation actually cares about reading config. So exposing it directly on the policy itself just bloats the policy with stuff users probably don't care about. Also having this in a separate Config class allows that class to be used internally in the builders, which is convenient.
• Event listener registration is still done directly on the policy class itself, in a threadsafe way
  • It's worth discussing whether this makes sense
• Policy metrics are accessible on policies themselves, for example, CircuitBreaker.getFailureCount()
• A shortcut for creating a RetryPolicy or CircuitBreaker with defaults exists via .ofDefaults()
• Shortcut methods for creating a Fallback or Timeout exist with just the required argument exist. For creating one with optional arguments, the builder must be used.

The policy SPI now lives in a .spi package and provides:

• A base PolicyExecutor for creating custom policies
• An AbstractPolicy implementation that implements ExecutionListeners
• FailurePolicy and DelayablePolicy implementations
• Other internal types that are exposed to policy implementors, including SyncExecutionInternal, AsyncExecutionInternal, FailsafeFuture, ExecutionResult

Fixes #299 Fixes #47 Fixes #201

[jhalterman]

@Tembrel @timothybasanov This PR introduces policy builders for 3.0. You can also browse the 3.0 branch to have a look: https://github.com/failsafe-lib/failsafe/tree/3.0

I'd appreciate your feedback to make sure I'm not missing anything and that this impl makes sense. See the initial post above for some notes on the impl.

[whiskeysierra]

Event listener registration is still done directly on the policy class itself, in a threadsafe way

What's the reasoning behind that?

[Tembrel]

This is a huge step forward! The only downside that I see is having to explicitly call build() to create a policy instance, but:

1. the presence of build() makes it clear that an instance is being created,
2. it's not that much more to type,
3. a few well-chosen static shortcut methods in the Failsafe API can anyway often reduce that extra typing,
4. the user can supply other static shortcut methods for frequently used build sequences, and
5. all of this is well worth the benefits of separating build phase from policy usage.

Now I can have more confidence that policies can be shared between threads safely. It should be made clear that builders themselves are not necessarily thread-safe (because they modify fields of Config instances with no synchronization), but their products are.

I had a moment of doubt when I saw that the implementations of getConfig() were returning direct references to the policy's Config field, but as long as there is no public mutability via the returned reference, it should be OK.

I don't have a problem with maintaining the event listeners as volatile members of the policy. They are copied to the policy executor at toExecutor time, so I suppose there is the danger of a race if other threads are modifying the policy at the same time. But I would call that user error: What do you mean by calling onAbort in one thread while using the policy in an execution in another thread?

[jhalterman]

What's the reasoning behind that? [having event listener registration on policies themselves]

I'm not 100% on this... these could go in the builders as well. I think it comes down to whether event listeners are likely to be configured when a policy is built or if they might, for good reason, be configured later. I'm interested for feedback on this.

One awkward part of having event listeners not be part of a builder, is it create an awkward API chain, where some configuration is done before build() and some afterwards:

RetryPolicy.builder()
  .withMaxRetries(3)
  .build()
  .onFailure(e -> System.out.println(e));

a few well-chosen static shortcut methods in the Failsafe API can anyway often reduce that extra typing,

Indeed, since Fallback and Timeout both have a required parameter, they offer static shortcut methods that don't require using the builder. The builder is only needed if you want to tweak some additional configuration.

It should be made clear that builders themselves are not necessarily thread-safe

I added a mention in each of the builder javadocs that "this class is not threadsafe", and to the config and policy javadocs that they are threadsafe.

[jhalterman]

What's the reasoning behind that? [having event listener registration on policies themselves]

@whiskeysierra One more follow up on this. I recalled my rationale for not wanting to put even listener registration in the builders was that you might want to register listeners after a policy is built, for example, if integrating a policy with some metrics/monitoring libraries (prometheus, micrometer, etc). Having to do this when the policy is being built could be a hassle. That said, there's no reason why listeners couldn't be registered in a builder or afterwards. Perhaps that's a decent path.

On a related note, I'm somewhat interested to see if any requests come in the future to be able to reconfigure policies that are already built. The one good use case I see for this is circuit breakers, where you might want to change a breaker's threshold dynamically to adapt to changing conditions, where replacing the breaker across your code is not easy to do. I'm happy to wait and see how necessary this is, post 3.0.

---

@Tembrel and others, there are a few other changes I wanted to see if you had any input on for 3.0, since now is probably the time to make these changes:

• I'd like to repackage everything to the dev.failsafe package. Having packaging that aligns with the project's domain, now that we have one, is nice. Any serious objections?
• I'm considering moving the policies to separate maven modules, but still providing an "all" dependency which will provide all the policies. Thoughts?

[whiskeysierra]

@whiskeysierra One more follow up on this. I recalled my rationale for not wanting to put even listener registration in the builders was that you might want to register listeners after a policy is built, for example, if integrating a policy with some metrics/monitoring libraries (prometheus, micrometer, etc). Having to do this when the policy is built could be a hassle.

I'd solve that differently. Instead of forcing this requirement on all policy implementations, I'd make the policies full immutable and the builders contain a single event listener. Then there could be a dedicated event listener implementation: a composite that takes a list of listeners and makes them look/behave like a single one. If you pass a mutable list to that one, you get mutable listeners. If you pass a thread-safe, mutable collection (e.g. CopyOnWriteArrayList), even better. (see https://whiskeysierra.github.io/knowledge/patterns/composite/)

[jhalterman]

@whiskeysierra any idea what that API might look like? The current API has a single registration method for each listener type: ex: onFailure(listener), which is nice and straightforward.

[whiskeysierra]

any idea what that API might look like? The current API has a single registration method for each listener type: ex: onFailure(listener), which is nice and straightforward.

RetryPolicy.builder()
    .onFailure(composite(listOf(
        MyListener(),
        MyOtherListener()
    )))
    .build()

[Tembrel]

• I'd like to repackage everything to the dev.failsafe package. Having packaging that aligns with the project's domain, now that we have one, is nice. Any serious objections?

Sounds good.

• I'm considering moving the policies to separate maven modules, but still providing an "all" dependency which will provide all the policies. Thoughts?

Seems unnecessarily fussy to me. What's the drawback to keeping them all together as one artifact? Are you worried that people will object to having to drag in more policies than they use? They all seem pretty lightweight to me, so that's never been one of my concerns.

[jhalterman]

Are you worried that people will object to having to drag in more policies than they use?

Yea, though I haven't actually heard that complaint yet, so it's fine to wait on that for now. It just occurred to me as an idea now that the policy impls are more decoupled from the rest of the core as part of the work to improve the SPI.

[jhalterman]

@whiskeysierra So then you could mutate listeners by accessing them from the config? Ex:

retryPolicy.getConfig().getFailureListeners().add(anotherFailureListener);

It seems more straightforward to just support onFailure in the builder or policy, or both.

[jhalterman]

I ended up moving the listeners into the builders and excluding them from the policies for now. Event listeners might be worth revisiting in the future, such as to support multiple listeners for an event type, but that shouldn't need to change anything here.

Feel free to post any additional comments here, but I'm going to merge this for now.