Add ApiVersions #80
Add ApiVersions #80
Conversation
apiversions.go
Outdated
| return | ||
| } | ||
|
|
||
| type ApiVersionsV1 struct { |
There was a problem hiding this comment.
This is a protocol type, it shouldn't be exported.
conn.go
Outdated
| @@ -989,6 +989,31 @@ func (c *Conn) requestHeader(apiKey apiKey, apiVersion apiVersion, correlationID | |||
| } | |||
| } | |||
|
|
|||
| // ApiVersions return the list of API versions supported by the broker. | |||
| func (c *Conn) ApiVersions() (versions []ApiVersionsV1, err error) { | |||
There was a problem hiding this comment.
Is there a reason why we would need to call this from a program? If not we shouldn't export it.
There was a problem hiding this comment.
I would say that it depends if the strategy is to let the user defining which protocol version to use or if we do that automatically in the library.
There was a problem hiding this comment.
Let's do it automatically, kafka-go aims to be simple to use, this kind of implementation complexity is worth abstracting.
conn.go
Outdated
| return []ApiVersionsV1{}, err | ||
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
You're mixing names and unnamed return values, this is a bit confusing to the reader.
docker-compose.yml
Outdated
| @@ -5,7 +5,7 @@ kafka: | |||
| ports: | |||
| - "9092:9092" | |||
| environment: | |||
| KAFKA_VERSION: "0.10.1.0" | |||
| KAFKA_VERSION: "0.11.0.1" | |||
There was a problem hiding this comment.
I double-checked and it looks like this environment variable isn't used anymore, we should use an explicit tag on the image instead.
There was a problem hiding this comment.
I think it's still in used : https://github.com/wurstmeister/kafka-docker/blob/master/Dockerfile#L8.
I've also just tested it to run the tests against Kafka 1.0.0.
Pryz
force-pushed
the
add-apiversions
branch
from
6460766
to
16ec167
Compare
Pryz
force-pushed
the
add-apiversions
branch
15 times, most recently
from
1e8f847
to
167e58e
Compare
Pryz
force-pushed
the
add-apiversions
branch
10 times, most recently
from
f56bbaa
to
9707e28
Compare
Pryz
force-pushed
the
add-apiversions
branch
2 times, most recently
from
fd4bc4e
to
7e5237f
Compare
Pryz
force-pushed
the
add-apiversions
branch
7 times, most recently
from
39ece72
to
ed09f3c
Compare
Pryz
force-pushed
the
add-apiversions
branch
5 times, most recently
from
9ab0fcd
to
c4afafa
Compare
|
I'm going to close this PR. The first idea was just to introduce the ability to use the ApiVersion API and it then became the "dealing with all the Kafka API versions". Instead of doing that, we are going to rethink the internals of the library and use the latest message formats. |
This PR adds the
ApiVersionsfunction to the API.