Skip to content

Commit 927990b

Browse files
Argonusclaude
andauthored
fix(metadata): keep leaderless partitions in cluster metadata (#595)
Metadata parsing dropped every partition the broker reported with an error code, so a partition whose leader was moving disappeared from the client's view. The default partitioner derives its partition count from map_size(partition_leaders), so that count shrank mid-move and the same key was remapped to a different partition — silent per-key ordering loss. Partitions carrying a leader-move error are now retained with the reported leader (-1 = leader unknown), PartitionInfo carries the broker's error_code, and ClusterMetadata.select_node/2 answers {:error, :leader_not_available} instead of resolving a negative node id to :broker_not_found. The no-broker lookup also logs its cause (unknown topic vs leaderless partition vs unknown node) at debug. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 3a18cbe commit 927990b

13 files changed

Lines changed: 190 additions & 33 deletions

File tree

CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,14 @@
1818
`base_offset: nil`. A failed async send closes the socket, and the client now handles
1919
`{:tcp_error}` / `{:ssl_error}` plus a catch-all `handle_info/2` instead of crashing on an
2020
unmatched message.
21+
* **A key could silently change partition during a leader move.** Metadata parsing dropped every
22+
partition the broker reported with an error, shrinking the count the default partitioner keys
23+
off — so the same key could land on a different partition. Partitions carrying a leader-move
24+
error are now retained with the reported leader (`-1` = leader unknown),
25+
`ClusterMetadata.select_node/2` answers `{:error, :leader_not_available}` for them, and
26+
`PartitionInfo` carries the broker's `error_code`.
27+
* **`:no_broker` says why in the log.** Unknown topic vs. leaderless partition vs. unknown node are
28+
now distinguished (logged at debug); the returned error is unchanged.
2129

2230
## 1.1.1 (2026-07-24)
2331

lib/kafka_ex/client/client.ex

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1187,15 +1187,27 @@ defmodule KafkaEx.Client do
11871187
updated_state = state_updater.(state)
11881188

11891189
case State.select_broker(updated_state, selector) do
1190-
{:error, _} -> {nil, updated_state}
1191-
{:ok, broker} -> ensure_broker_connected(broker, updated_state)
1190+
{:error, reason} ->
1191+
# Debug, not warning: the consumer's fetch loop retries this lookup and owns the throttled log.
1192+
Logger.debug("No broker for #{describe_selector(selector)} after metadata refresh: #{inspect(reason)}")
1193+
1194+
{nil, updated_state}
1195+
1196+
{:ok, broker} ->
1197+
ensure_broker_connected(broker, updated_state)
11921198
end
11931199

11941200
{:ok, broker} ->
11951201
ensure_broker_connected(broker, state)
11961202
end
11971203
end
11981204

1205+
defp describe_selector(%NodeSelector{strategy: :topic_partition, topic: topic, partition: partition}),
1206+
do: "#{topic}/#{partition}"
1207+
1208+
defp describe_selector(%NodeSelector{strategy: :consumer_group, consumer_group_name: group}),
1209+
do: "consumer group #{group}"
1210+
11991211
# Ensures broker is connected, reconnecting if necessary.
12001212
# Returns {broker, updated_state} where broker may have a new socket,
12011213
# or nil if reconnection failed.

lib/kafka_ex/cluster/cluster_metadata.ex

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ defmodule KafkaEx.Cluster.ClusterMetadata do
2323
@typedoc """
2424
Possible errors given by `select_node/2`
2525
"""
26-
@type node_select_error :: :no_such_node | :no_such_topic | :no_such_partition | :no_such_consumer_group
26+
@type node_select_error ::
27+
:no_such_node | :no_such_topic | :no_such_partition | :no_such_consumer_group | :leader_not_available
2728

2829
@doc """
2930
List names of topics known by the cluster metadata
@@ -78,6 +79,7 @@ defmodule KafkaEx.Cluster.ClusterMetadata do
7879
{:ok, %Topic{partition_leaders: partition_leaders}} ->
7980
case Map.fetch(partition_leaders, partition) do
8081
:error -> {:error, :no_such_partition}
82+
{:ok, node_id} when node_id < 0 -> {:error, :leader_not_available}
8183
{:ok, node_id} -> {:ok, node_id}
8284
end
8385
end

lib/kafka_ex/cluster/partition_info.ex

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,17 @@ defmodule KafkaEx.Cluster.PartitionInfo do
88
Java equivalent: `org.apache.kafka.common.PartitionInfo`
99
"""
1010

11-
defstruct partition_id: nil, leader: -1, replicas: [], isr: []
11+
alias Kayrock.ErrorCode
12+
13+
# Informational only: node selection keys off `leader` (< 0 = no leader), not this field.
14+
defstruct partition_id: nil, leader: -1, replicas: [], isr: [], error_code: :no_error
1215

1316
@type t :: %__MODULE__{
1417
partition_id: integer(),
1518
leader: integer(),
1619
replicas: [integer()],
17-
isr: [integer()]
20+
isr: [integer()],
21+
error_code: atom()
1822
}
1923

2024
@doc """
@@ -23,7 +27,8 @@ defmodule KafkaEx.Cluster.PartitionInfo do
2327
## Parameters
2428
2529
The metadata map should contain:
26-
- `:error_code` - Must be 0 for successful parsing
30+
- `:error_code` - The broker's partition error code (0 = no error), retained as-is. Unlike the
31+
production `parse_partitions/1`, this constructor applies no error-code whitelist
2732
- `:partition` - The partition number
2833
- `:leader` - The leader node ID
2934
- `:replicas` - List of replica node IDs
@@ -32,7 +37,7 @@ defmodule KafkaEx.Cluster.PartitionInfo do
3237
"""
3338
@spec from_partition_metadata(map()) :: t()
3439
def from_partition_metadata(%{
35-
error_code: 0,
40+
error_code: error_code,
3641
partition_index: partition,
3742
leader_id: leader,
3843
replica_nodes: replicas,
@@ -42,7 +47,8 @@ defmodule KafkaEx.Cluster.PartitionInfo do
4247
partition_id: partition,
4348
leader: leader,
4449
replicas: replicas,
45-
isr: isr
50+
isr: isr,
51+
error_code: ErrorCode.code_to_atom(error_code)
4652
}
4753
end
4854

lib/kafka_ex/cluster/topic.ex

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,14 @@ defmodule KafkaEx.Cluster.Topic do
2323
partitions: partition_metadata,
2424
is_internal: is_internal
2525
}) do
26-
partition_leaders =
27-
Enum.into(
28-
partition_metadata,
29-
%{},
30-
fn %{error_code: 0, leader_id: leader, partition_index: partition_id} ->
31-
{partition_id, leader}
32-
end
33-
)
34-
3526
partitions = Enum.map(partition_metadata, &PartitionInfo.from_partition_metadata/1)
3627

28+
# Keep leaderless partitions (leader -1); dropping them shrinks the count the partitioner keys off.
29+
partition_leaders =
30+
Enum.into(partitions, %{}, fn %PartitionInfo{partition_id: id, leader: leader} ->
31+
{id, leader}
32+
end)
33+
3734
%__MODULE__{
3835
name: name,
3936
partition_leaders: partition_leaders,

lib/kafka_ex/protocol/kayrock/metadata/response_helpers.ex

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,17 +85,32 @@ defmodule KafkaEx.Protocol.Kayrock.Metadata.ResponseHelpers do
8585
@spec parse_partitions([map()]) :: [PartitionInfo.t()]
8686
def parse_partitions(kayrock_partitions) when is_list(kayrock_partitions) do
8787
kayrock_partitions
88-
|> Enum.filter(&(ErrorCode.code_to_atom(&1.error_code) == :no_error))
89-
|> Enum.map(fn partition_map ->
88+
|> Enum.map(&{ErrorCode.code_to_atom(&1.error_code), &1})
89+
|> Enum.filter(fn {error, _} -> usable_partition?(error) end)
90+
|> Enum.map(fn {error, partition_map} ->
9091
%PartitionInfo{
9192
partition_id: partition_map.partition_index,
9293
leader: partition_map.leader_id,
9394
replicas: partition_map.replica_nodes || [],
94-
isr: partition_map.isr_nodes || []
95+
isr: partition_map.isr_nodes || [],
96+
error_code: error
9597
}
9698
end)
9799
end
98100

101+
# A partition whose leader is moving comes back with an error code and leader
102+
# -1. Dropping it makes it indistinguishable from a partition that does not
103+
# exist, and shrinks the partition count the partitioner keys off. Keep it and
104+
# let node selection report :leader_not_available. Mirrors kpro's
105+
# discover_partition_leader/4 and librdkafka's NULL broker delegation.
106+
defp usable_partition?(:no_error), do: true
107+
defp usable_partition?(:leader_not_available), do: true
108+
defp usable_partition?(:replica_not_available), do: true
109+
defp usable_partition?(:not_leader_for_partition), do: true
110+
defp usable_partition?(:not_leader_or_follower), do: true
111+
defp usable_partition?(:kafka_storage_error), do: true
112+
defp usable_partition?(_), do: false
113+
99114
@doc """
100115
Checks if the metadata response contains any errors.
101116
"""

test/kafka_ex/api_test.exs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,13 @@ defmodule KafkaEx.APITest do
308308
assert [{:topic_metadata, ["test-topic"]}, {:produce, "test-topic", _partition, ^messages}] = calls
309309
end
310310

311+
test "counts a partition whose leader is moving, so keys keep their partition" do
312+
settled = %Topic{name: "test-topic", partition_leaders: %{0 => 1, 1 => 2, 2 => 1}}
313+
moving = %Topic{name: "test-topic", partition_leaders: %{0 => 1, 1 => -1, 2 => 1}}
314+
315+
assert partition_for(settled, "user-123") == partition_for(moving, "user-123")
316+
end
317+
311318
test "returns error when topic has no partitions (nil partition)" do
312319
topic_info = %Topic{name: "test-topic", partition_leaders: %{}}
313320

@@ -821,4 +828,17 @@ defmodule KafkaEx.APITest do
821828
assert {:error, :invalid_consumer_group} = KafkaEx.API.set_consumer_group_for_auto_commit(client, nil)
822829
end
823830
end
831+
832+
defp partition_for(topic_info, key) do
833+
{:ok, client} =
834+
MockClient.start_link(%{
835+
topic_metadata: {:ok, [topic_info]},
836+
produce: {:ok, %RecordMetadata{topic: topic_info.name, partition: 0, base_offset: 0}}
837+
})
838+
839+
{:ok, _} = KafkaEx.API.produce(client, topic_info.name, nil, [%{key: key, value: "data"}])
840+
841+
[_, {:produce, _, partition, _}] = MockClient.get_calls(client)
842+
partition
843+
end
824844
end
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
defmodule KafkaEx.Client.NoBrokerReasonTest do
2+
@moduledoc """
3+
`:no_broker` collapses three different causes — unknown topic, unknown
4+
partition, unknown node — so the reason has to reach the log, otherwise a
5+
leaderless partition is indistinguishable from a deleted topic.
6+
"""
7+
use ExUnit.Case, async: false
8+
9+
import ExUnit.CaptureLog
10+
11+
alias KafkaEx.Client
12+
alias KafkaEx.Client.NodeSelector
13+
alias KafkaEx.Client.State
14+
alias KafkaEx.Cluster.ClusterMetadata
15+
alias KafkaEx.Cluster.Topic
16+
17+
defp state_with(topics) do
18+
%State{cluster_metadata: %ClusterMetadata{brokers: %{}, topics: topics}}
19+
end
20+
21+
defp fetch_from(state, topic, partition) do
22+
request = %Kayrock.Fetch.V0.Request{replica_id: -1, max_wait_time: 1, min_bytes: 1, topics: []}
23+
selector = NodeSelector.topic_partition(topic, partition)
24+
25+
capture_log(fn ->
26+
{:reply, reply, _} = Client.handle_call({:network_request, request, selector}, self(), state)
27+
send(self(), {:reply, reply})
28+
end)
29+
end
30+
31+
test "names the missing partition behind :no_broker" do
32+
state = state_with(%{"t" => %Topic{name: "t", partition_leaders: %{0 => 1}}})
33+
34+
log = fetch_from(state, "t", 7)
35+
36+
assert_received {:reply, {:error, :no_broker}}
37+
assert log =~ "No broker for t/7"
38+
assert log =~ ":no_such_partition"
39+
end
40+
41+
test "names the missing topic behind :no_broker" do
42+
log = fetch_from(state_with(%{}), "gone", 0)
43+
44+
assert_received {:reply, {:error, :no_broker}}
45+
assert log =~ "No broker for gone/0"
46+
assert log =~ ":no_such_topic"
47+
end
48+
end

test/kafka_ex/cluster/cluster_metadata_test.exs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,4 +396,23 @@ defmodule KafkaEx.Cluster.ClusterMetadataTest do
396396
assert ClusterMetadata.known_topics(updated_cluster) == ["topic-one"]
397397
end
398398
end
399+
400+
describe "select_node/2 with a leaderless partition" do
401+
test "reports :leader_not_available rather than a bogus node id" do
402+
metadata = %ClusterMetadata{
403+
brokers: %{1 => %KafkaEx.Cluster.Broker{node_id: 1, host: "h", port: 9092}},
404+
topics: %{"t" => %KafkaEx.Cluster.Topic{name: "t", partition_leaders: %{0 => 1, 1 => -1}}}
405+
}
406+
407+
alias KafkaEx.Client.NodeSelector
408+
409+
assert {:ok, 1} = ClusterMetadata.select_node(metadata, NodeSelector.topic_partition("t", 0))
410+
411+
assert {:error, :leader_not_available} =
412+
ClusterMetadata.select_node(metadata, NodeSelector.topic_partition("t", 1))
413+
414+
assert {:error, :no_such_partition} =
415+
ClusterMetadata.select_node(metadata, NodeSelector.topic_partition("t", 9))
416+
end
417+
end
399418
end

test/kafka_ex/cluster/partition_info_test.exs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,21 @@ defmodule KafkaEx.Cluster.PartitionInfoTest do
3939

4040
assert partition.isr == [21]
4141
end
42+
43+
test "maps a zero error_code to :no_error", %{metadata: metadata} do
44+
partition = PartitionInfo.from_partition_metadata(metadata)
45+
46+
assert partition.error_code == :no_error
47+
end
48+
49+
test "retains a leader-move partition instead of crashing, mapping its error_code" do
50+
metadata = %{error_code: 5, partition_index: 0, leader_id: -1, replica_nodes: [], isr_nodes: []}
51+
52+
partition = PartitionInfo.from_partition_metadata(metadata)
53+
54+
assert partition.leader == -1
55+
assert partition.error_code == :leader_not_available
56+
end
4257
end
4358

4459
describe "build/1" do

0 commit comments

Comments
 (0)