Refactor and test the 0.8.0 implementation #264
Conversation
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
| @@ -47,6 +51,21 @@ defmodule KafkaEx.Server do | |||
| ssl_options: KafkaEx.ssl_options, | |||
| use_ssl: boolean | |||
| } | |||
|
|
|||
| @spec increment_correlation_id(t) :: t | |||
| def increment_correlation_id(state = %State{correlation_id: cid}) do | |||
There was a problem hiding this comment.
File has the variable name before the pattern while most of the files have the variable name after the pattern when naming parameter pattern matches
lib/kafka_ex/server.ex
Outdated
| request.topic, | ||
| request.partition | ||
| ) | ||
|
|
There was a problem hiding this comment.
There should be no trailing white-space at the end of a line.
lib/kafka_ex/server.ex
Outdated
| wire_request = request | ||
| |> client_request(updated_state) | ||
| |> module.create_request | ||
|
|
There was a problem hiding this comment.
There should be no trailing white-space at the end of a line.
lib/kafka_ex/server.ex
Outdated
| config_sync_timeout() | ||
| ) | ||
| |> module.parse_response | ||
|
|
There was a problem hiding this comment.
There should be no trailing white-space at the end of a line.
lib/kafka_ex/server.ex
Outdated
| |> module.parse_response | ||
|
|
||
| state_out = State.increment_correlation_id(updated_state) | ||
|
|
There was a problem hiding this comment.
There should be no trailing white-space at the end of a line.
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/kafkaex/kafka_ex/pulls/264. |
GenServer.stop was added in 1.2
There was a problem hiding this comment.
I don't know if it's worth addressing the 0.8 SSL issue at all, given that we will be dropping 0.8 support soon one way or another.
The refactor seems reasonable to me. I think we might want to take a look at testing against the correct broker versions when we move on to refactor 0.9 and add 0.10/0.11/1.0
|
Can we get this one merged soon, if it is ok? |
|
@ashwini709 I will try to get it merged tomorrow if @bjhaid has no objections. Do you mind if I ask for your motivation for this? The reason I’m asking is that we are assessing if anyone is using the 0.8.x implementations. See #260 |
|
@dantswain We are using Kafka version 0.11.0, But I could see relevant file changes that might conflict with #235 done for #233 |
My ultimate goal is to first move to Protocol implementations for the wire requests and then ultimately support the various message format changes with version changes (see #261, #245). To make these changes more manageable, I want to do some fairly heavy refactoring of the server implementations, so that they are better factored to reason about and share more implementation at the base level.
I started with the 0.8.0 implementation because it is the simplest. Even though we are discussing dropping support for it (see #260), I think this is not wasted effort since it allows me to start with the simplest implementation and sort out some of the basic issues.
The way that I am testing this could be controversial. I am connecting the 0.8.0 implementation to the 0.9.0 test broker with an SSL connection. 0.8.0 does not support SSL so this required allowing the 0.8.0 implementation to attempt a connection with SSL. I am emitting a warning message when the user attempts to do this. The only alternative that I could think of was to actually spin up a 0.8.0 cluster in test, which would significantly increase the complexity of the test setup and the duration of the test run on travis.