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

[Issue #897] Flush on unbatched producer panics #898

Conversation

flowchartsman
Copy link
Contributor

Fixes #897

Am not certain if this is the right place or behavior, so opening it up for comments. It might be that it is fine to panic on a Flush() to a non-batching producer, provided it's explicitly documented, but probably not. It's worth noting that it worked in the past, which is how I ran into the issue in the first place, since I left an errant Flush() call behind in some tests after disabling batching on a producer.

Master Issue: #

Motivation

No one wants a panic, especially one that is undocumented.

Modifications

nil check.

Verifying this change

  • Make sure that the change passes the CI checks.

I've included tests.

Documentation

Since this issue would normally cause a panic, it's probably not necessary to document anything special, especially considering that a similar check is made elsewhere throughout the code. If requested, I can add a comment to the Flush() Godoc mentioning that it is a no-op for non-batching producers.

Copy link
Contributor

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe the formatting changes should be a separate PR since they are not related to the bug.

Copy link
Member

@shibd shibd left a comment

Choose a reason for hiding this comment

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

@flowchartsman Can you remove invalid formatting changes?

@shibd shibd changed the title [Issue #897] [Issue #897] Flush on unbatched producer panics Mar 15, 2023
@dimovnike
Copy link

it might be that it is fine to panic on a Flush() to a non-batching producer

may be it is but it is still better if an error is returned, not panic.

@flowchartsman
Copy link
Contributor Author

This issue was duplicated by #1064 (I guess no one searches issues any more) and fixed by #1065. Closing this as superceded.

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.

Flush on unbatched producer panics
4 participants