Add a "create topics" action #297

jbruggem · 2018-07-18T09:39:05Z

TODO:

rebase and rename my commits
clean up code to make it integrate well with existing code
add missing error codes ?
check compliance with https://github.com/kafkaex/kafka_ex/blob/master/CONTRIBUTING.md

A new server version was added because CreateTopics was added in 0.10.1 (see also the release notes).

Closes #168

sourcelevel-bot · 2018-07-18T09:39:11Z

Hello, @jbruggem! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

joshuawscott · 2018-10-03T13:35:01Z

@jbruggem are you ready for review on this yet? Feel free to ping me when you are.

jbruggem · 2018-10-03T13:43:34Z

Thanks for offering!

It's actually probably relevant to get a first review. There's probably already a lot of stuff you can point out that will help push this through.

jbruggem · 2018-10-03T13:45:03Z

I should probably rebase first, though ?

joshuawscott · 2018-10-03T13:46:20Z

Rebasing would be ideal, yes. I can go ahead and look it over once the conflict is resolved.

Thanks!

jbruggem · 2018-10-03T13:53:17Z

I've rebased it.

I'm not sure if I've managed to follow the spirit of the existing code correctly in all circumstances. In particular, I had to add the notion of API version in the parsing of metadata and it felt like I didn't do it cleanly enough.

thanks again for the review.

bjhaid

Thanks for this! Can you confirm if 0.9.0 supports create topic if it does then you probably don't need a new server type, also you need to add something like this:

kafka_ex/lib/kafka_ex/server_0_p_8_p_0.ex

Line 56 in f374cfb

    
           def kafka_server_offset_fetch(_, _state), do: raise "Offset Fetch is not supported in 0.8.0 version of kafka"

to all the servers not supporting create_topic, so a user gets a useful error message if they try using it for a wrongly configured server

lib/kafka_ex/config.ex

bjhaid · 2018-10-03T14:08:34Z

lib/kafka_ex/server.ex

+
+      def update_metadata(state, api_version) do
+        {correlation_id, metadata} = retrieve_metadata_with_version(state.brokers, state.correlation_id, config_sync_timeout(), api_version)
+        metadata_brokers = metadata.brokers |> Enum.map(&(%{&1 | is_controller: &1.node_id == metadata.controller_id}))


is this change necessary for you PR can you indicate why?

We had to add the api_version argument to functions handling metadata because in the Metadata API response object version 0 does not provide the controller_id value, which is needed to create a topic.

Therefore, it was necessary to allow support for the version 1 of the Metadata API, hence the need to specify the version in the function call.

jbruggem · 2018-10-03T14:17:56Z

@bjhaid

Can you confirm if 0.9.0 supports create topic if it does then you probably don't need a new server type

A new server version was added because CreateTopics exists only since 0.10.1 (see also the release notes).

add something like this [...] to all the servers not supporting create_topic,

I already did that, but maybe not the way I should have ?

bjhaid · 2018-10-03T14:21:58Z

I already did that, but maybe not the way I should have ?

Sorry my bad, you did the right thing!

bjhaid · 2018-10-03T14:42:01Z

lib/kafka_ex/protocol/metadata.ex

@@ -2,6 +2,8 @@ defmodule KafkaEx.Protocol.Metadata do
  alias KafkaEx.Protocol
  import KafkaEx.Protocol.Common

+  @default_api_version 0


After your previous comment wondering if we should create a new metadata_v1 module? I have not thought deeply about this just throwing it out as I imagine the more versions of things we support to more clunky the constructing request and parsing of response may become...

I think it's a relevant question. That's indeed the kind of question that require a deep understanding of this project, which I'm not sure I have yet.

Especially since 0.10.0.0, the client is supposed to ask the broker which API versions are supported (see "Retrieving Supported API versions"). The "broker returns its full list of supported ApiKeys and versions", meaning KafkaEx should use that information to decide which API message and version to use (and allow)

This means that for version >= 0.10.0.0, KafkaEx's behaviour should be quite different than the current hard-coded mapping of versions -> code.

I'll leave the decision on how to proceed to @joshuawscott and @dantswain both of whom have been more involved in the project more recently than I have, however I think we should have different version modules and after looking up the supported api version either reject the api-version provided by the user or pick the newest one we have a module for and use the module... last time I looked I think there are a couple of versions of the requests that will require some amount of work to implement them, we need as much help as we can get :)

I'm wondering if maybe this discussion should happen in another issue, so that we can move forward with this merge request. It seems the scale is much bigger than just adding support for a new message in the protocol. I'd be happy to help, of course !

Would that be OK ?

@dantswain @joshuawscott what do yall think?

So, it wouldn't be accurate to have a metadata_v1 module, since the API version is per individual API message. For example, CreateTopics could be v3, but the same server could accept SyncGroup v2. Each of the message types needs to be able to generate messages based on the API version(s) that the broker indicates are available.

@joshuawscott agreed.

scripts/docker_up_ubuntu.sh

scripts/ci_tests.sh

* all tests pass locally * added a docker_up script for ubuntu * fix mistakes in kafka_create_topics definition * fix test command in ci_tests

jbruggem · 2018-10-08T20:19:49Z

All tests pass locally (142 tests, 0 failures -- I've run them many times), but not on the CI. I suppose there are race conditions which become visible in that context. I tried to increase the number of retries, to no avail.

sourcelevel-bot · 2018-10-08T20:35:29Z

Ebert has finished reviewing this Pull Request and has found:

3 possible new issues (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/kafkaex/kafka_ex/pulls/297.

joshuawscott · 2018-10-09T14:21:54Z

I restarted the two tests that failed (elixir 1.1 & elixir 1.5)

jbruggem · 2018-10-09T14:39:34Z

Thanks !

joshuawscott

I left some comments, but I did not get all the way through the PR unfortunately. I might not be able to touch this for a couple of days, but feel free to disagree with any of the points I raised.
Cheers!

.travis.yml

lib/kafka_ex/protocol/create_topics.ex

joshuawscott · 2018-10-09T14:37:54Z

lib/kafka_ex/protocol/create_topics.ex

+  end
+
+  @spec encode_nullable_string(String.t) :: binary
+  defp encode_nullable_string(text) do


nullable string could be a nil, but the spec doesn't allow that. This function could use pattern matching instead of an if as well.

nullable string could be a nil, but the spec doesn't allow that.

So I should leave it as it is ?

This function could use pattern matching instead of an if as well.

I'll replace the if with pattern matching.

by 'spec' I meant the typespec of the function: encode_nullable_string should take nil | String.t

joshuawscott · 2018-10-09T14:42:49Z

lib/kafka_ex/protocol/metadata.ex

@@ -2,6 +2,8 @@ defmodule KafkaEx.Protocol.Metadata do
  alias KafkaEx.Protocol
  import KafkaEx.Protocol.Common

+  @default_api_version 0


So, it wouldn't be accurate to have a metadata_v1 module, since the API version is per individual API message. For example, CreateTopics could be v3, but the same server could accept SyncGroup v2. Each of the message types needs to be able to generate messages based on the API version(s) that the broker indicates are available.

joshuawscott · 2018-10-09T14:47:38Z

lib/kafka_ex/protocol/metadata.ex

@@ -93,18 +97,44 @@ defmodule KafkaEx.Protocol.Metadata do
    }
  end

-  def create_request(correlation_id, client_id, ""), do: KafkaEx.Protocol.create_request(:metadata, correlation_id, client_id) <> << 0 :: 32-signed >>
+  def valid_api_version(v) do


I'm not sure that we need this function, since we could just have the api_version arguments default to @default_api_version

Ok, I'll fix that

lib/kafka_ex/protocol/produce.ex

joshuawscott · 2018-10-09T15:07:27Z

lib/kafka_ex/protocol/metadata.ex

@@ -113,13 +143,55 @@ defmodule KafkaEx.Protocol.Metadata do
    parse_brokers(brokers_size - 1, rest, [%Broker{node_id: node_id, host: host, port: port} | brokers])
  end

+  defp parse_brokers_v1(0, rest, brokers), do: {brokers, rest}


Attaching version numbers to the functions will become cumbersome, I think. Many of the APIs have several versions, but those version might not have any differences, which might lend itself better to having one a function parameter that is the API version.

For example, Metadata Response has no differences in the brokers field from versions 1-6.

Indeed. I think it's going to become cumbersome either way, to be honest :).

I've switched to using a function parameter instead. Tell me what you think !

lib/kafka_ex/server.ex

joshuawscott · 2018-10-09T15:14:16Z

lib/kafka_ex/server.ex

@@ -378,17 +396,27 @@ defmodule KafkaEx.Server do

      # credo:disable-for-next-line Credo.Check.Refactor.FunctionArity
      def retrieve_metadata(brokers, correlation_id, sync_timeout, topic \\ []), do: retrieve_metadata(brokers, correlation_id, sync_timeout, topic, @retry_count, 0)
+
+      # credo:disable-for-next-line Credo.Check.Refactor.FunctionArity
+      def retrieve_metadata_with_version(brokers, correlation_id, sync_timeout, api_version, topic \\ []), do: retrieve_metadata(brokers, correlation_id, sync_timeout, topic, @retry_count, api_version, 0)


We should just add api_version as the last argument to retrieve_metadata and default it to @default_api_version (0). No need to add another function.

joshuawscott · 2018-10-09T15:15:09Z

lib/kafka_ex/server.ex

      # credo:disable-for-next-line Credo.Check.Refactor.FunctionArity
      def retrieve_metadata(_, correlation_id, _sync_timeout, topic, 0, error_code) do
        Logger.log(:error, "Metadata request for topic #{inspect topic} failed with error_code #{inspect error_code}")
        {correlation_id, %Metadata.Response{}}
      end
+
+      # credo:disable-for-next-line Credo.Check.Refactor.FunctionArity
+      def retrieve_metadata(brokers, correlation_id, sync_timeout, topic, retry, error_code) do


this could be removed by using a default parameter as suggested above

jbruggem · 2018-10-10T19:32:51Z

Ok, I addressed all comments the best I could. Tests pass locally (not always on Travis).

* add a new target to Travis CI's config * fix an error in docker_up_ubuntu

… in parse_brokers

joshuawscott

Just a couple more minor things that I see, and then it looks good

Thanks so much for taking this on!

joshuawscott · 2018-10-10T23:49:59Z

test/integration/server0_p_10_p_1_test.exs

+
+    resp = KafkaEx.create_topics([request], timeout: 2000)
+    # error = TOPIC_ALREADY_EXISTS
+    assert {36, name} == parse_create_topic_resp(resp)


We should be able to add 36 to the @error_map in lib/kafka_ex/protocol.ex and then refer to this as the error code (:topic_already_exists) rather than 36

Good idea, thanks ! I've added a few error codes, might as well :).

lib/kafka_ex/protocol/metadata.ex

jbruggem · 2018-10-12T13:10:20Z

OK, I think this is now ready for merge !

joshuawscott · 2018-10-12T13:12:29Z

I'm going to wait until the end of the day to let @bjhaid or @dantswain weigh in if they wish, then I'll merge.

Thanks again!

jbruggem · 2018-10-12T13:49:04Z

Thanks again!

My pleasure! We were happy to be able to contribute to a nice open-source project, and it was very useful for us to have this feature implemented. I'm open to the idea of further contributions of course.

jbruggem · 2018-10-15T14:33:22Z

awesome 🎉. Thanks for all your help !

jbruggem mentioned this pull request Jul 18, 2018

Add support for CreateTopics API #168

Closed

jbruggem force-pushed the add_create_topic_action branch from 5a5494d to d0300b4 Compare July 18, 2018 11:59

jbruggem force-pushed the add_create_topic_action branch from 82acb5b to f374cfb Compare October 3, 2018 13:48

bjhaid reviewed Oct 3, 2018

View reviewed changes

sourcelevel-bot bot reviewed Oct 8, 2018

View reviewed changes

scripts/docker_up_ubuntu.sh Show resolved Hide resolved

sourcelevel-bot bot reviewed Oct 8, 2018

View reviewed changes

scripts/docker_up_ubuntu.sh Show resolved Hide resolved

jbruggem force-pushed the add_create_topic_action branch from 78cb301 to 16e9a0f Compare October 8, 2018 18:38

sourcelevel-bot bot reviewed Oct 8, 2018

View reviewed changes

scripts/ci_tests.sh Show resolved Hide resolved

sourcelevel-bot bot reviewed Oct 8, 2018

View reviewed changes

scripts/ci_tests.sh Show resolved Hide resolved

jbruggem added 6 commits October 8, 2018 22:11

Add preliminary support for CreateTopics API

0a621d6

Prevent memory leak in message producer when request is invalid

254429e

Add network timeout artgument to create_topics

994ee30

Set 0.10.1 as the new default version

ba31542

Fix failing tests and correct errors in kafka_create_topics

e54f9b6

* all tests pass locally * added a docker_up script for ubuntu * fix mistakes in kafka_create_topics definition * fix test command in ci_tests

Update dialyxir to get a much nicer UX; fix warnings

9f15c7f

jbruggem force-pushed the add_create_topic_action branch from 95e91ad to db9b487 Compare October 8, 2018 20:19

jbruggem changed the title ~~Add a "create topics" action (work in progress)~~ Add a "create topics" action Oct 8, 2018

jbruggem force-pushed the add_create_topic_action branch from 061ac11 to f9bf497 Compare October 8, 2018 20:34

jbruggem force-pushed the add_create_topic_action branch from ae47859 to c6c4b6e Compare October 8, 2018 21:02

joshuawscott reviewed Oct 9, 2018

View reviewed changes

jbruggem force-pushed the add_create_topic_action branch from c6c4b6e to d3f384e Compare October 10, 2018 17:32

jbruggem added 6 commits October 10, 2018 22:55

Add longer timeouts in tests + misc

0fa8353

* add a new target to Travis CI's config * fix an error in docker_up_ubuntu

create_topics: replace if with case and fix errenous spec

8dad507

Reorder functions in produce.ex

ca40764

Remove function valid_api_version in protocol/metadata.ex

50bccf4

Use a function parameter for api_version instead of the function name…

aa56408

… in parse_brokers

Remove function retrieve_metadate_with_version

af4a79a

jbruggem force-pushed the add_create_topic_action branch from 8f4cd86 to af4a79a Compare October 10, 2018 20:55

joshuawscott approved these changes Oct 11, 2018

View reviewed changes

jbruggem added 2 commits October 11, 2018 12:54

Consolidate create_request signatures in metadata.ex

75ea8a3

Change type of TopicError.error_code from integer to atom

9befa79

joshuawscott merged commit 10699a4 into kafkaex:master Oct 15, 2018

jbruggem deleted the add_create_topic_action branch October 15, 2018 14:32

Add a "create topics" action #297

Add a "create topics" action #297

Conversation

jbruggem commented Jul 18, 2018 • edited Loading

sourcelevel-bot bot commented Jul 18, 2018

joshuawscott commented Oct 3, 2018

jbruggem commented Oct 3, 2018

jbruggem commented Oct 3, 2018

joshuawscott commented Oct 3, 2018

jbruggem commented Oct 3, 2018

bjhaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbruggem commented Oct 3, 2018

bjhaid commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbruggem commented Oct 8, 2018 • edited Loading

sourcelevel-bot bot commented Oct 8, 2018

joshuawscott commented Oct 9, 2018

jbruggem commented Oct 9, 2018

joshuawscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbruggem commented Oct 10, 2018 • edited Loading

joshuawscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbruggem commented Oct 12, 2018

joshuawscott commented Oct 12, 2018

jbruggem commented Oct 12, 2018

jbruggem commented Oct 15, 2018

jbruggem commented Jul 18, 2018 •

edited

Loading

jbruggem commented Oct 8, 2018 •

edited

Loading

jbruggem commented Oct 10, 2018 •

edited

Loading