NetworkClient should not send nil in case of failure #275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
lib/kafka_ex/server_0_p_8_p_2.ex
Outdated
| response = broker | ||
| |> NetworkClient.send_sync_request(offset_commit_request_payload, config_sync_timeout()) | ||
| |> OffsetCommit.parse_response | ||
| response = case NetworkClient.send_sync_request(broker, offset_commit_request_payload, config_sync_timeout()) do |
There was a problem hiding this comment.
Line is too long (max is 80, was 116).
ashwini709
force-pushed
the
network_client
branch
3 times, most recently
from
4fee1be to
5ca5132
Compare
lib/kafka_ex/server.ex
Outdated
| @@ -526,7 +532,10 @@ defmodule KafkaEx.Server do | |||
| defp first_broker_response(request, brokers, sync_timeout) do | |||
| Enum.find_value(brokers, fn(broker) -> | |||
| if Broker.connected?(broker) do | |||
| NetworkClient.send_sync_request(broker, request, sync_timeout) | |||
| case NetworkClient.send_sync_request(broker, request, sync_timeout) do | |||
There was a problem hiding this comment.
Function body is nested too deep (max depth is 2, was 3).
lib/kafka_ex/server.ex
Outdated
| @@ -526,7 +532,10 @@ defmodule KafkaEx.Server do | |||
| defp first_broker_response(request, brokers, sync_timeout) do | |||
| Enum.find_value(brokers, fn(broker) -> | |||
| if Broker.connected?(broker) do | |||
| NetworkClient.send_sync_request(broker, request, sync_timeout) | |||
| case NetworkClient.send_sync_request(broker, request, sync_timeout) do | |||
There was a problem hiding this comment.
Function body is nested too deep (max depth is 2, was 3).
lib/kafka_ex/server.ex
Outdated
| @@ -304,8 +304,12 @@ defmodule KafkaEx.Server do | |||
| 0 -> NetworkClient.send_async_request(broker, produce_request_data) | |||
| _ -> | |||
| response = broker | |||
| |> NetworkClient.send_sync_request(produce_request_data, config_sync_timeout()) | |||
| |> Produce.parse_response | |||
| |> NetworkClient.send_sync_request(produce_request_data, config_sync_timeout()) | |||
There was a problem hiding this comment.
Line is too long (max is 80, was 95).
lib/kafka_ex/server.ex
Outdated
| @@ -334,8 +338,12 @@ defmodule KafkaEx.Server do | |||
| {:topic_not_found, state} | |||
| _ -> | |||
| response = broker | |||
| |> NetworkClient.send_sync_request(offset_request, config_sync_timeout()) | |||
| |> Offset.parse_response | |||
| |> NetworkClient.send_sync_request(offset_request, config_sync_timeout()) | |||
There was a problem hiding this comment.
Line is too long (max is 80, was 87).
lib/kafka_ex/server.ex
Outdated
| do: module.parse_response(response), | ||
| else: nil | ||
| response = broker | ||
| |> NetworkClient.send_sync_request(wire_request, config_sync_timeout()) |
There was a problem hiding this comment.
Line is too long (max is 80, was 85).
lib/kafka_ex/server.ex
Outdated
| @@ -526,7 +532,10 @@ defmodule KafkaEx.Server do | |||
| defp first_broker_response(request, brokers, sync_timeout) do | |||
| Enum.find_value(brokers, fn(broker) -> | |||
| if Broker.connected?(broker) do | |||
| NetworkClient.send_sync_request(broker, request, sync_timeout) | |||
| case NetworkClient.send_sync_request(broker, request, sync_timeout) do | |||
There was a problem hiding this comment.
Line is too long (max is 80, was 82).
ashwini709
force-pushed
the
network_client
branch
from
5ca5132 to
b4b8a00
Compare
joshuawscott
reviewed
Mar 3, 2018
lib/kafka_ex/server_0_p_8_p_2.ex
Outdated
| config_sync_timeout()) | ||
| |> case do | ||
| {:error, reason} -> {:error, reason} | ||
| response -> OffsetFetch.parse_response(response) |
There was a problem hiding this comment.
I think this should be OffsetCommit.parse_response?
There was a problem hiding this comment.
Yup. My Bad. I have updated the PR. Thanks.
ashwini709
force-pushed
the
network_client
branch
from
b4b8a00 to
7e85668
Compare
ashwini709
force-pushed
the
network_client
branch
from
7e85668 to
38c8633
Compare
|
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/275. |
|
@joshuawscott When can we get this merged? |
|
Going to give the other maintainers a chance to weigh in and then merge tomorrow U.S. time |
bjhaid
approved these changes
Mar 5, 2018
dantswain
approved these changes
Mar 5, 2018
|
Thanks again for the PR @ashwini709 ! |
robotarmy
pushed a commit
to RAM9/kafka_ex
that referenced
this pull request
Apr 7, 2025
NetworkClient should not send nil in case of failure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As pointed out by @dantswain Swallowing
nilin #272 was not a good idea. It leads to further crashes in hereresponse |> hd |> Map.get(:partitions) |> hd |> Map.get(:last_offset)If we return the
{:error, reason}in case of network_client'ssend_sync_reuqestfailure, it leads to the genserver crashing but with a valid reason.I would like to fix the issues related to #233 as much as possible.
@joshuawscott @dantswain would appreciate the guidance/suggestions to fix this behaviour. Thank you.
genconsumer's consume/1handle the result and reply with error?While we are at it, I would like to send
{:ok, response}in case of success if thats ok.