kafkaex / kafka_ex

Kafka client library for Elixir
MIT License
596 stars 162 forks source link

Wrong contract for KafkaEx.create_topics #410

Closed VitorTrin closed 4 years ago

VitorTrin commented 4 years ago

The documentation says that it receives a KafkaEx.Protocol.CreateTopics.Request struct, except the only valid input is a list of KafkaEx.Protocol.CreateTopics.TopicRequest structs.

joshuawscott commented 4 years ago

Good catch, we'd welcome a PR to fix it.

VitorTrin commented 4 years ago

Should we enforce the usage of Request or simply change the documentation? Because as it is Request has no use and just sending a list is simpler. On the other hand, sending a Request an receiving a Response seems to be a design decision

jbruggem commented 4 years ago

I checked the code, and if I'm correct CreateTopics.Request is used internally to translate the request into Kafka's binary protocol. It's not meant to be used by users of KafkaEx.

So indeed, it's a question of changing the documentation.

What I don't understand is that the spec already looks correct, so I can't figure out what I'm missing.

In kafka_ex.ex:

# ...
# ...
# ...
alias KafkaEx.Protocol.CreateTopics.TopicRequest, as: CreateTopicsRequest
# ...
# ...
# ...
# ...
# ...
  @spec create_topics([CreateTopicsRequest.t()], Keyword.t()) ::
          CreateTopicsResponse.t()
  def create_topics(requests, opts \\ []) do
    worker_name = Keyword.get(opts, :worker_name, Config.default_worker())
    timeout = Keyword.get(opts, :timeout)
    Server.call(worker_name, {:create_topics, requests, timeout})
  end
jbruggem commented 4 years ago

The contract is correct if I generate the doc on my machine:

image

On hexdocs:

image

@joshuawscott any idea what would cause the difference ?

joshuawscott commented 4 years ago

@jbruggem looks like it's been fixed in master already at some point. I'm going to close this; we probably do need to cut a new release, though.