group: reset all partitions if invoked with -partition -1

[disq]

I'm new to Kafka (long time Kinesis user) so take this PR with a grain of salt :)
I thought that maybe you wouldn't want an implicit match-all-partitions logic in a potentially destructive (well, write) operation, so came up with the "-1" magic number. Maybe the -partition param can be a string, then it would accept all for all partitions etc.

[disq]

Not really sure about this help message.

[fgeller]

thanks a lot @disq !

i'm not working much with multi-partitioned topics so wasn't sure what the use cases are around resetting the consumer groups for multiple partitions. so thanks for providing one!

• i think your suggestion to use an explicit -partition all parameter would be nicer - what do you think? i would probably keep the -1 as an "internal" flag, but use "all" as the user parameter, if that makes sense?

• do you think extending the -partition parameter to also accept a comma separated list of partitions would make sense? not necessarily as part of this pr, just asking for use cases in general.

• i'm curious: what race were you running into that you fixed in a927247?

[disq]

Yes, I think implementing an all param would be clearer for users. Also, for specifying multiple (well, a list of) partitions, one could invoke with the -partition parameter multiple times maybe, something like kt -group g -topic t -partition 1 -partition 3 -partition 17? What do you think?

The race is, since the cmd.reset variable was getting overwritten in L189, if invoked with -reset oldest or -reset newest, groupOff and pom.MarkOffset calls are potentially made with offsets from other partitions. So not all partitions were getting set to correct offsets.

[disq]

Few more suggestions - maybe specify the partition ID in the offset resolve message in L192, and rename the rst param to something that makes sense like resolvedOffset etc.

[fgeller]

@disq sorry, slow to respond at the moment - traveling.

• are you interested in adding the -partition all bit, or do you want me to merge as is and then extend?
• i think i'd prefer a single -partition argument, would parallel e.g. the -offsets parameter of the consume command if that makes sense?
• interpolating the partition argument into the log message in L192 makes sense to me!
• agreed, would also prefer resolvedOffset over rst

[disq]

I'll do the -partition all bit and the param rename, and leave you with the multiple-partitions-param stuff. Haven't checked the code in detail, but it might need significant refactoring for that one.

[disq]

Haven't had the time to test it yet, I'll report back if there's something to fix.

[disq]

Tested, seems to work.

[fgeller]

cheers @disq !