Skip to content

Conversation

ebati
Copy link
Contributor

@ebati ebati commented Oct 6, 2018

This PR updates dependencies, use markoffset/resetoffset depending on offset, use version parsing from sarama.

Please don't merge. I will try to write some test for offset management but i don't have time to do it now.

By the way, i also used failf function but it might not be OK, since it exits with os.Exit which exits immediately without calling deferred functions calls.

return dflt
v, err := sarama.ParseKafkaVersion(strings.TrimPrefix(s, "v"))
if err != nil {
failf(err.Error())
Copy link
Owner

Choose a reason for hiding this comment

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

just a quick note: can you make sure these are legible to a cli user? ie, does the error message give you enough context that the user would know that the version string they're passing isn't correct/supported? also, changing this means that we generally want to support all versions that sarama supports - i think that's not true yet, or? cf #83

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if version regex velidation fail, ParseKafkaVersion return invalid version %s as error, we can add more context if you want.

Since we use sarama for operations on kafka, we should be able to support all versions, sarama supports. Kafka 2.0 is not working in master since sarama 1.12 does not support it.

Copy link
Owner

@fgeller fgeller Oct 25, 2018

Choose a reason for hiding this comment

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

if version regex velidation fail, ParseKafkaVersion return invalid version %s

alright, sounds good to me.

Kafka 2.0 is not working in master since sarama 1.12 does not support it.

alright, i would like to make sure that happens anyway, so let's pull this update in and then update the tests on master to run against newer versions of kafka.

@fgeller
Copy link
Owner

fgeller commented Oct 6, 2018

thanks for splitting the work up into small commits 🙏

@fgeller
Copy link
Owner

fgeller commented Oct 25, 2018

@ebati i'll pull this into master now, and will try to find some time to work on that test myself, let me know if you think you'll have time in the next days and prefer to do that bit yourself. thanks for your work and time!

@fgeller fgeller merged commit d9ec6df into fgeller:master Oct 25, 2018
@ebati
Copy link
Contributor Author

ebati commented Oct 25, 2018

I am so sorry, I couldn't find time to continue working on this. You can take. Thanks for this great tool ☺️

@fgeller
Copy link
Owner

fgeller commented Oct 25, 2018

no worries, cheers!

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