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

Update doc of ConsumerSettings#withBlocker #583

Merged
merged 2 commits into from
Apr 10, 2021

Conversation

bplommer
Copy link
Member

@bplommer bplommer commented Apr 8, 2021

See #582

@bplommer bplommer requested review from vlovgr and LMnet April 8, 2021 09:36
@LMnet
Copy link
Member

LMnet commented Apr 8, 2021

Do we really need to deprecate it? Maybe we could just add some additional information about requirements about thread-pool in the scaladoc. Because custom thread-pool may be useful for custom error handling, for logging, or to have good thread names.

@bplommer
Copy link
Member Author

bplommer commented Apr 8, 2021

Do we really need to deprecate it? Maybe we could just add some additional information about requirements about thread-pool in the scaladoc. Because custom thread-pool may be useful for custom error handling, for logging, or to have good thread names.

I don't have a particularly strong opinion on this. It does raise another question though - if this is an ability we want to keep, should we also allow users of 2.x to provide custom ExecutionContexts for blocking operations?

@LMnet
Copy link
Member

LMnet commented Apr 8, 2021

should we also allow users of 2.x to provide custom ExecutionContexts for blocking operations?

I think we should.

@bplommer
Copy link
Member Author

bplommer commented Apr 8, 2021

I think we should.

I'm fine with that. So we'd have withCustomBlockingContext(ec: ExecutionContext) in the Settings classes (I think the word custom should be there because it's confusing if blockingContext can be None when a blocking context is always required). The fact that we have a Blocking abstraction should make it pretty easy to do, as in #584.

@LMnet
Copy link
Member

LMnet commented Apr 8, 2021

Sounds good for me.

@bplommer bplommer changed the title deprecate ConsumerSettings#withBlocker Update doc of ConsumerSettings#withBlocker Apr 10, 2021
@bplommer
Copy link
Member Author

I've revised this PR to just update the doc - I'll go ahead and merge.

@bplommer bplommer merged commit 945c624 into series/1.x Apr 10, 2021
@bplommer bplommer deleted the deprecate-consumersettings-withblocker branch April 10, 2021 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants