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

Split consumer’s Events() channel into Messages() and Errors() #303

Merged
merged 5 commits into from
Feb 24, 2015

Conversation

wvanbergen
Copy link
Contributor

This is more in line with the producer. Also, it is really easy to forget checking for the Err field for every ConsumerEvent, which would lead to bugs.

@Shopify/kafka

}

return nil
}

// ConsumerEvent is what is provided to the user when an event occurs. It is either an error (in which case Err is non-nil) or
// ConsumerMessage is what is provided to the user when an event occurs. It is either an error (in which case Err is non-nil) or
Copy link
Contributor

Choose a reason for hiding this comment

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

the remainder of this godoc is now out-of-date

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

a few little things (and one suggestion of an additional enhancement) but nothing major

…an consumer example that works with goroutines instead of a select statement.
@wvanbergen wvanbergen mentioned this pull request Feb 24, 2015
4 tasks
@wvanbergen
Copy link
Contributor Author

Should I guard multiple calls to AsyncClose() with a sync.Once to prevent panics?

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

Nope, I think that's overly-defensive

Err error
}

func (ce ConsumerError) Error() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% convinced this needs to implement error itself, given we never (ourselves) actually stuff it into an error type, but I guess it doesn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nice part is that it will add some context to the error message, e.g. when printing the errors that you consume from the Errors() channel, and it can be passed around as error value.

    for err := range consumer.Errors() {
        fmt.Println(err)
    }

Primarily, I think it's expected that an Errors() channel will give you error instances

@wvanbergen
Copy link
Contributor Author

e.g., to ensure something like this doesn't cause issues:

    wg.Add(1)
    go func() {
        defer wg.Done()
        for err := range consumer.Errors() {
            consumer.AsyncClose()
        }
    }()

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

I'd consider that just bad code o.O

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

Pretty sure it's not legit to call Close more than once on any of the standard library IO constructs.

@wvanbergen
Copy link
Contributor Author

I guess you can just break the for loop, and call (Async)Close outside the loop. It may leave some errors undrained but that is not really an issue.

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

You can just call Close outside the loop and it will drain/return any remaining errors.

@wvanbergen
Copy link
Contributor Author

OK, I think I addressed all the outstanding issues.

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

LGTM

wvanbergen pushed a commit that referenced this pull request Feb 24, 2015
Split consumer’s Events() channel into Messages() and Errors()
@wvanbergen wvanbergen merged commit cc01cb4 into master Feb 24, 2015
@wvanbergen wvanbergen deleted the split_consumer_events_channel branch February 24, 2015 20:28
@eapache eapache added this to the 1.0.0 milestone Mar 5, 2015
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