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

Make instantiation of CommitTimeoutException lazy. #1150

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

optician
Copy link
Contributor

Hi!
I lost about 1 day trying to figure out why there are a lot of CommitTimeoutException in jfr files (with stacktrace) and zero entries in application logs. So I suggest making it lazy to save time for others.

@bplommer
Copy link
Member

Thanks! That sounds really annoying - thanks for raising this. I'm a bit unsure about proceeding with this change though.

On the one hand, it does seem like the change would improve user experience in cases like yours.

On the other hand, this seems to be a much more general issue with how cats raiseError and cats-effect timeoutTo work, so possibly something that should be raised in those upstream libraries. I'm not sure they'd accept this as being a problem, as instantiating an exception isn't a side effect - on the other hand, generating a stack trace isn't free, so maybe they should.

Actually, the point about generating a stack trace not being free makes me more inclined to go ahead with merging this. I'll give it some thought and might raise the general issue on TypeLevel discord in the meantime.

@optician
Copy link
Contributor Author

optician commented Mar 27, 2023

I don't like stacktrace performance in general, but in this case it does not seem to be a problem. I have a service with very limited resources, and the impact of stacktraces is negligible. With the default batch settings. I think this would be a problem with tiny batches, but kafka hits the performance hard for them anyway. So it's all about UX imho.

I doubt raiseError would be changed because it's a breaking change. Also cats-effect has similar code. I think this is intentional behaviour. Perhaps this has been discussed already.


upd.
Strange gitlab search. Manual search shows 3 results but the link shows 0 with the same string.
example: https://github.com/typelevel/cats-effect/blob/73a379dad0480c9885c717fbaf27dc6ad779193d/core/shared/src/main/scala/cats/effect/IO.scala#L740

@aartigao aartigao changed the base branch from series/2.x to series/3.x September 29, 2023 18:58
Copy link
Contributor

@aartigao aartigao left a comment

Choose a reason for hiding this comment

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

Given that even CE authors use this pattern and it will be better for performance (even as a microoptimization), I think this deserves to be merged ✅

Thank you for your contribution! 🎉 🙇🏽

@aartigao aartigao merged commit 0774c76 into fd4s:series/3.x Sep 29, 2023
8 checks passed
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.

3 participants