Add ApiVersions request #160
Conversation
achille-roussel
left a comment
achille-roussel
left a comment
There was a problem hiding this comment.
Looking good overall, just left a couple of questions.
| } | ||
|
|
||
| if errorCode != 0 { | ||
| return r, Error(errorCode) |
There was a problem hiding this comment.
Should we return a nil []ApiVersion here? Or is there value in exposing a partial result?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Should we put a default value here?
There was a problem hiding this comment.
I guess I can put the value that lists all the versions that the current implementation of kafka-go works with.
There was a problem hiding this comment.
Seems like it could help making good use of this field 👍
conn.go
Outdated
|
|
||
| // number of replica acks required when publishing to a partition | ||
| requiredAcks int32 | ||
| apiVersions []ApiVersion |
There was a problem hiding this comment.
Is there a guarantee on the ordering of the values here? I'm wondering how the lookups will work.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
A map makes sense to me 👍
|
@achille-roussel, I addressed the comments. |
|
Looking good, thanks for adding this! |
2 participants
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.