Rework or remove CommitRecovery

[LMnet]

Recently I have worked with the CommitRecovery functionality. And I found that this part of the library is not pleasant to work with.

In particular, I faced the next issues:

1. recoverCommitWith signature has specific bounds on F. And there is no way to add some extra bounds if I want. For example, I wanted to use log4cats and add some details about retrying inside recoverCommitWith. And at the current moment, I can't that because recoverCommitWith doesn't have a Sync bound. Overall recoverCommitWith's signature feels weird and limiting.
2. CommitRecovery.Default is pretty nice, but not configurable.
3. I'm using pureconfig for my configuration and I had to create a custom wrapper around CommitRecovery to make it works nicely with the pureconfig.

Also, It's debatable that this functionality should be built-in inside fs2-kafka. For me, it looks like all this retries stuff could be delegated to a specialized solution like cats-retry. And it may look more natural and composable. ATM this part is not composable: I had to overcome some difficulties to use CommitRecovery with the pureconfig, log4cats, and cats-retry.

I can suggest 2 ways of solving this:

1. Completely remove this functionality. If the user will want to retry commit failures he can use any library and add retries on top of the commit method. As a bonus point here we will be able to remove Jitter from the library, because it uses only in CommitRecovery logic.
2. Somehow rework this functionality. Maybe remove CommitRecovery logic from the KafkaConsumerActor and offer a CommitRecovery as an additional option on top of a commit method?

[bplommer]

Sorry, I only just saw this - had my github notifications set up wrong  🤦‍♂️ I'll try give this some thought in the next few days.

[bplommer]

I feel like removing commit recovery as a default behaviour would be a regression in terms of robustness and approachability, so I'd like to avoid that if possible.

What if we convert CommitRecovery to be tagless final, so the bounds on F can be implementation-specific? It would mean you need Sync to instantiate ConsumerSettings with the default CommitRecovery but I don't see that as a huge problem. I think that should address your composability issue, though I'm not sure if it would address the PureConfig issue.

2. Somehow rework this functionality. Maybe remove CommitRecovery logic from the KafkaConsumerActor and offer a CommitRecovery as an additional option on top of a commit method?

I don't quite understand what you're suggesting. But CommitRecovery could potentially be passed to the actor as part of the commit request instead of via settings - would this help with your config issue?

[LMnet]

What if we convert CommitRecovery to be tagless final, so the bounds on F can be implementation-specific? It would mean you need Sync to instantiate ConsumerSettings with the default CommitRecovery but I don't see that as a huge problem. I think that should address your composability issue, though I'm not sure if it would address the PureConfig issue.

This will definitely not help with the pureconfig, because pureconfig implies that config creation is pure. And it sounds pretty reasonable for me — config is just pure data, why should I need some effects for creating it?

I don't quite understand what you're suggesting. But CommitRecovery could potentially be passed to the actor as part of the commit request instead of via settings - would this help with your config issue?

I suggest removing CommitRecovery from the actor logic and move it to the commit method. Like this:

def commit(recovery: CommitRecovery = CommitRecovery.Default): F[Unit]

Also, I found a few other options:

3. Simplify CommitRecovery to a boolean flag. And make the interface itself private. If the user will have some custom logic, he will have to disable commit recovery and add his custom solution.
4. Make CommitRecovery.Default configurable and move its config to a ConsumerSettings. Like this:

case class CommitRecoveryConfig(
  enabled: Boolean,
  maxAttempts: Int,
  maxRetryingTime: FiniteDuration
)

And make the CommitRecovery interface private too.

The main idea here is to hide the CommitRecovery interface because it's too limiting. In case of any custom need, users will have to disable CommitRecovery by a boolean flag and add any logic he wants.

I don't think that we need to evolve CommitRecovery to something more useful and complex. There are other libraries specific to that.

[bplommer]

Ok, your first suggestion sounds reasonable to me. What do you think @vlovgr?

[vlovgr]

I agree on removing CommitRecovery and Jitter, but would like to keep the default functionality with an option to disable or configure it.

[LMnet]

@vlovgr just to clarify — it's option 3, right?

[bplommer]

I suggest removing CommitRecovery from the actor logic and move it to the commit method. Like this:

def commit(recovery: CommitRecovery = CommitRecovery.Default): F[Unit]

This was the one I was in favour of - @vlovgr were you thinking this or reducing it to a boolean flag?

[vlovgr]

That option sounds good to me. 👍

[LMnet]

@vlovgr I'm sorry, but I'm still don't understand which is your favorite option.

Thinking a bit about this recently and I concluded that the boolean flag is the best option. Moving CommitRecovery to a commit method (option 2) will not solve problems with the CommitRecovery itself. It will still be a poor implementation of retry logic.

[vlovgr]

Long-term (at least new major release) we should get rid of CommitRecovery completely and use a boolean flag (or some basic customization options). But short-term, we're a bit more limited, hence moving it to commit sounds like a reasonable alternative.