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

Add documentation for handling rebalance #150

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Z1kkurat
Copy link
Contributor

No description provided.

README.md Outdated Show resolved Hide resolved
README.md Outdated
which requires you to return a `RebalanceCallback` structure which describes what actions should be performed in a certain situation.
It allows you to use some of consumer's methods as well as a way to run an arbitrary computation.
Please note that all the actions are executed on the consumer `poll` thread which means that running heavy or
long-running computations is discouraged.
Copy link
Contributor

Choose a reason for hiding this comment

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

heavy or long-running computations is discouraged.
I think we need to add some short explanation why is it discouraged and what are the options if it's really really neded :)

  • discouraged coz if it takes too long then kafka consumer instance can be removed from consumer group (default is 5 minutes and then consumer instance considered dead roughly speaking), but most probably before those 5 default minutes we would fail with timeout exception (default is 1 minute there) in ToTry
  • so to workaround those limitations user can adjust ToTry/max.poll.interval.ms timeouts

I realise now it's really hard to explain such important mechanics in fewer words, so mb it deservs a link to a dedicated section about ToTry/max.poll.interval.ms timeouts?

More details are available in following issues:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some clarifications - please check now, will that be enough?

README.md Outdated

What you can currently do:
- lift a pure value via `RebalanceCallback.pure(a)`
- raise an error via `RebalanceCallback.fromTry(Failure(error))`
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked at #146 yet, but mb we would be able to raise error via ApplicativeError after merging it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We surely would. But it would be a bit strange if this is merged before #146 and has a notion of functionality that isn't merged yet, no? If this is merged before #146 I could fix the doc in the corresponding PR

Copy link
Contributor

@nikitapecasa nikitapecasa May 17, 2021

Choose a reason for hiding this comment

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

sure, didn't mean to create any confusion, sorry :)
I'm ok with any of the following options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a short description regarding raising errors as well as handling. Please check if that will be enough

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Z1kkurat Z1kkurat marked this pull request as ready for review May 17, 2021 15:24
@Z1kkurat
Copy link
Contributor Author

Since #146 is merged, I will make a note about error handling in the docs

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