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

kgo: export the wrapped error from ErrGroupSession #789

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sbuliarca
Copy link
Contributor

We want to test in a unit test the behaviour exposed in this question: #784, where we rely on this error type. So we need to create this kind of error, with another wrapped one inside it in the tests.

Would this change be ok? The only side efect of it is that clients of the lib would be able to create it too.

@twmb
Copy link
Owner

twmb commented Jul 24, 2024

Why not use Unwrap to get the inner error, rather than exposing the error?

@sbuliarca
Copy link
Contributor Author

Why not use Unwrap to get the inner error, rather than exposing the error?

Our problem is that we want to create this error ourselves, in the test, to simulate a kgo.Fetches that contains this error. Something like:

egs := kgo.ErrGroupSession{Err: syscall.ECONNREFUSED}
fetch := kgo.Fetch{
			Topics: []kgo.FetchTopic{{
				Topic: "dummyTopic",
				Partitions: []kgo.FetchPartition{{
					Partition: 0,
					Err:       err,
				}},
			}}}

We currently can not do it, as the err is not exported in kgo.ErrGroupSession.

@sbuliarca
Copy link
Contributor Author

Hey @twmb! Just a friendly reminder of the existence of this PR. Thanks!

@twmb
Copy link
Owner

twmb commented Aug 27, 2024

I haven't forgotten -- I'm primarily waiting until this weekend or next (or next) to set aside time to implement the three new KIPs that were released in Kafka 3.8.

That said -- I'm still not sure what value a test on this particular error provides? Stakes here are pretty low though, so I'm not really opposed either.

@sbuliarca
Copy link
Contributor Author

I'm still not sure what value a test on this particular error provides?

For us, it allows to unit test the exact scenario described in the question #784 , that we're not exiting the poll loop when we get such an error.

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.

2 participants