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 IsTimeout method to Error #903

Merged
merged 1 commit into from
Nov 28, 2022
Merged

Conversation

milindl
Copy link
Contributor

@milindl milindl commented Nov 24, 2022

This convenience method just checks if Error code is ErrTimedOut.

Also modifies the example in README.md.

See #846 (comment)

@milindl milindl requested a review from a team November 24, 2022 09:23
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Add a CHANGELOG entry.
Also recommend updating one of the existing tests to use this too:
find a test that checks Code() == ..TimedOut,
in that test also add IsTimeout().

Why keep the old way? Because we want to make sure that we're not breaking existing functionality.

kafka/error.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -295,6 +295,9 @@ func TestConsumerAssignment(t *testing.T) {
if err.(Error).Code() != ErrTimedOut {
t.Errorf("Expected ReadMessage to fail with ErrTimedOut, not %v", err)
}
if !err.(Error).IsTimeout() {
t.Errorf("Expected ReadMessage to fail with a timeout error, not %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a case for the success case too. E.g., from the TestConsumerAPIs dry-test (doesn't require a broker), you could call ReadMessage() with a low timeout and verify it is a timeout.

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'm a bit unsure if I got this right since the case in TestConsumerAssignment is also a success case (ie it's a timeout).
I added a test in TestConsumerAPIs, hopefully that captures the intent of this comment.

This convenience method just checks if Error code is
ErrTimedOut/ErrTimedOutQueue.

Also modifies the example in README.md.

Modifies the CHANGELOG.md.
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

LGTM

@milindl milindl merged commit 921537f into confluentinc:master Nov 28, 2022
PrasanthV454 pushed a commit that referenced this pull request Jan 12, 2023
This convenience method just checks if Error code is
ErrTimedOut/ErrTimedOutQueue.

Also modifies the example in README.md.

Modifies the CHANGELOG.md.
PrasanthV454 pushed a commit that referenced this pull request Mar 17, 2023
This convenience method just checks if Error code is
ErrTimedOut/ErrTimedOutQueue.

Also modifies the example in README.md.

Modifies the CHANGELOG.md.
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