Add ApiVersions request #160
Conversation
Looking good overall, just left a couple of questions.
| } | ||
|
|
||
| if errorCode != 0 { | ||
| return r, Error(errorCode) |
Should we return a nil []ApiVersion here? Or is there value in exposing a partial result?
My approach was, - I'm giving you whatever I have just use it at your own discretion since there was an error. I can return nil here if you think it makes more sense.
Up to you, I don't have strong feelings about it.
conn.go
Outdated
| var err error | ||
| c.apiVersions, err = c.ApiVersions() | ||
| if err != nil { | ||
| c.apiVersions = nil |
Should we put a default value here?
I guess I can put the value that lists all the versions that the current implementation of kafka-go works with.
Seems like it could help making good use of this field
conn.go
Outdated
| @@ -71,6 +71,7 @@ type Conn struct { | |||
|
|
|||
| // number of replica acks required when publishing to a partition | |||
| requiredAcks int32 | |||
| apiVersions []ApiVersion | |||
Is there a guarantee on the ordering of the values here? I'm wondering how the lookups will work.
I can check for ordering. But I just realized that for our purposes it, probably, makes more sense to put it in a map: map[api key]ApiVersion
A map makes sense to me
|
@achille-roussel, I addressed the comments. |
|
Looking good, thanks for adding this! |
I would like to merge this as a separate PR for easier review. I will need this change is implementing v2 message reader: #146
I'll be using this request right after connecting to kafka cluster in order to determine which fetch requests I'm allowed to send.
So far the plan is to implement fetch request v5. Fetch request v5 seems to have been introduced in kafka 0.11.0.0. And presumably this is the first version where server returns message sets in v2 format.
So, if fetch request v5 is available I'll be sending v5 otherwise v2.
The text was updated successfully, but these errors were encountered: