Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

breaking: Do not run user rebalance listener on same thread runtime #1205

Merged
merged 7 commits into from
Apr 3, 2024

Conversation

svroonland
Copy link
Collaborator

@svroonland svroonland commented Mar 30, 2024

⚠️ this change breaks compatibility. All fields in RebalanceListener are now of type Set[TopicPartition] => Task[Unit] (this used to be (Set[TopicPartition], RebalanceConsumer) => Task[Unit]).

Since #1098 the rebalance listener runs on a special same-thread-runtime. This is also true for the user provided rebalance listener. The same-thread-runtime is not convenient to use at all; a lot of care must be taken to keep the zio fiber on the same thread. Note, the only reason for using the same-thread-runtime is to enforce the same-thread access that is required by the kafka consumer (see #1098 for more details). However, the kafka consumer that is passed to the rebalance listener should be used via the internal ConsumerAccess.

Since #1098 it is no longer necessary to use the rebalance listener to commit offsets, this is done with the rebalanceSafeCommits feature. We do not see other use cases for the kafka consumer from inside the rebalance listener.

For these reasons, this change runs user provided rebalance listener on the regular runtime again. In addition, access to the kafka consumer from within the user rebalance listeners is removed.

@erikvanoosten
Copy link
Collaborator

Can we say this is an 'addition' iso of a 'correction'? 😉

@svroonland svroonland marked this pull request as ready for review March 30, 2024 11:00
@erikvanoosten
Copy link
Collaborator

We need to change some documentation as well.

@erikvanoosten erikvanoosten self-requested a review March 30, 2024 11:28
@erikvanoosten
Copy link
Collaborator

For example we can remove this remark in RebalanceListener:

Note that the given ZIO effects are executed directly on the Kafka poll thread. Fork and shift to another executor when this is not desired.

@erikvanoosten
Copy link
Collaborator

erikvanoosten commented Mar 30, 2024

I remember now why I didn't do this in the context of #1098. This is because running on another executor means you can't use the kafka consumer, at least not the one that is passed in. You have to use ConsumerAccess which is an internal API.

I see these options:

  1. We keep as is, and properly document that you have to be careful because you're running on a restricted executor.
  2. We go back to pre-1098 state, where users can't use the consumer that is given to the rebalance listener.
  3. Same, but we make ConsumerAccess available to the rebalance listener.
  4. We update this PR such that we don't even pass in the consumer into the rebalance listener.

@erikvanoosten erikvanoosten self-requested a review March 30, 2024 15:29
@svroonland
Copy link
Collaborator Author

Ah that is good to know. I would be in favour of #4. Any downsides to that, use cases where users explicitly want to commit offsets? I suppose that is now supported by rebalanceSafeCommits

@erikvanoosten
Copy link
Collaborator

Ah that is good to know. I would be in favour of #4.

Me as well.

Any downsides to that, use cases where users explicitly want to commit offsets? I suppose that is now supported by rebalanceSafeCommits

Exactly. What else could someone be doing in the rebalance listener that requires the consumer?

Copy link
Member

@guizmaii guizmaii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🙂

@svroonland svroonland merged commit 7b9f3c2 into master Apr 3, 2024
14 checks passed
@svroonland svroonland deleted the rebalance-listener-not-on-single-thread-executor branch April 3, 2024 05:44
@erikvanoosten erikvanoosten changed the title Do not run user rebalance listener on same thread runtime breaking: Do not run user rebalance listener on same thread runtime Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants