-
Notifications
You must be signed in to change notification settings - Fork 103
WIP: Update sarama #85
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
Conversation
return dflt | ||
v, err := sarama.ParseKafkaVersion(strings.TrimPrefix(s, "v")) | ||
if err != nil { | ||
failf(err.Error()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
thanks for splitting the work up into small commits 🙏 |
@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! |
I am so sorry, I couldn't find time to continue working on this. You can take. Thanks for this great tool |
no worries, cheers! |
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 withos.Exit
which exits immediately without calling deferred functions calls.