fix(produce, consumer): honour acks, and survive leader moves & SSL broker restarts without killing the group - #590
Open
Argonus wants to merge 8 commits into
Open
fix(produce, consumer): honour acks, and survive leader moves & SSL broker restarts without killing the group#590Argonus wants to merge 8 commits into
Argonus wants to merge 8 commits into
Conversation
…work
0.x translated `required_acks` onto the wire in the legacy adapter; the 1.0
rewrite removed that adapter and the translation with it, while
`KafkaEx.API.produce/5` kept documenting the option. The request builder reads
`:acks`, so since 1.0 every produce went out with `acks: -1` regardless of what
the caller asked for.
The option is now resolved once in the client (`:acks` > deprecated
`:required_acks` > -1) and feeds both the wire and the produce telemetry, so the
two can no longer disagree. Values other than -1/0/1 are rejected with
`{:error, :invalid_acks}` instead of reaching Kayrock's int16 encoder, where
`:all` raised inside the client and 65_536 silently truncated to 0 while the
client waited for a response that never came.
`acks: 0` was unreachable and, once reachable, crashed on `byte_size/1` over the
async send result. It now takes a dedicated path that skips the retry layer (a
resend would duplicate records, since the broker sends nothing to confirm the
first one) and returns `%RecordMetadata{base_offset: nil}`, which widens the
typespec to `non_neg_integer() | nil`.
Two failure modes that fire-and-forget makes routine are fixed alongside it: a
failed async send now closes the broken socket the way the sync send already
did, and `KafkaEx.Client` grows `{:tcp_error, _, _}` / `{:ssl_error, _, _}` plus
a catch-all `handle_info/2` — defining any clause replaces the one `use
GenServer` injects, so a connection reset (how a broker reports a rejected
acks=0 batch) killed the client along with every socket and all metadata.
Log unmatched client messages at warning level. The catch-all was added so an unexpected message no longer kills the client, but debug level put it below the default threshold — a late broker response arriving on an active socket was being discarded with no trace at all. Stop the acks=0 integration tests from racing partition leader election. Before this branch, required_acks: 0 silently meant acks: -1, so those produces were synchronous and retried; now they are genuinely fire-and-forget and a batch sent before the leader is elected is dropped without an error. Establish the leader with an acknowledged produce first, and poll for the offset instead of sleeping a fixed 500 ms.
A socket error handed :ssl_error's raw term to the close event, which the contract fixes to a small set of atoms — an alert tuple there breaks handlers that match known reasons and inflates metric cardinality. Also covers the send_async_request/2 error branch, which was untested.
GenConsumer stopped on any fetch error other than :offset_out_of_range, including ones Retry.transient_error?/1 already classifies as transient. With ConsumerGroup supervising at max_restarts: 0, a single partition losing its leader took the whole group down. RF=1 makes the window wide enough to hit reliably, but the same path runs on any leader move. Retries are unbounded with jittered exponential backoff, as in brod, KafkaJS and librdkafka. The backoff is held as a deadline rather than a bare GenServer timeout, because any inbound message re-arms that callback with timeout 0 and would otherwise cut the wait short.
:no_broker collapses :no_such_topic, :no_such_partition and :broker_not_found into one atom, so a leaderless partition reads the same as a deleted topic. The reason now reaches the log with the topic and partition; the returned error is unchanged, so this is safe for a patch release.
parse_partitions/1 dropped every partition the broker flagged, so a partition whose leader was moving became indistinguishable from one that does not exist. Two consequences: node selection reported :no_such_partition, and the produce partitioner - which sizes itself from map_size(partition_leaders) - silently remapped keys to other partitions while the count was short. Partitions carrying a leader-move error are kept with the reported leader (usually -1) and tagged with the broker's error code; select_node/2 answers :leader_not_available for them. Mirrors kpro's discover_partition_leader/4 and librdkafka's NULL broker delegation.
Three changes, all from review of the commits above.
Retry.backoff_delay/3 computed base * :math.pow(2, attempt) before applying the
cap. Every previous caller was bounded at 3-5 attempts; the new fetch loop is
not, and the float overflows into ArithmeticError past attempt 1023 - roughly 85
minutes of continuous failure at the 5s cap. It would have killed the consumer,
and with max_restarts: 0 the group, which is what this work exists to prevent.
Now an exact integer shift with a clamped exponent.
handle_offset_out_of_range/1 hard-matched {:ok, offset} from
latest_offset/earliest_offset. Those need a live leader too, so a leader move
during an offset reset raised MatchError. The failure now feeds the same backoff.
The startup path (load_offsets/1) still raises by design; documented as such.
The retry warning is throttled after the backoff reaches its cap.
Act on the code review of the leaderless-partition fix.
- A non-atom transport reason (an SSL {:tls_alert, _} tuple) was normalised to
:unknown, which the fetch loop treats as fatal — so on TLS clusters a broker
restart still took the whole group down, defeating the retry fix. Such reasons
now normalise to a retryable :transport_error.
- Retries stay unbounded (brod/Java/librdkafka do the same), but after
:fetch_unavailable_warn_ms (default 30s) of continuous failure a partition is
surfaced once via Logger.error and a [:kafka_ex, :consumer, :partition_unavailable]
telemetry event. Mirrors librdkafka's topic.metadata.propagation.max.ms.
- An acks=0 async send failure emitted a :send_error close event while the
active-mode {:tcp_error} handler emitted a second :recv_error for the same
connection death; the network layer no longer emits, leaving the handler the
single owner.
- The per-lookup "no broker after refresh" log drops to debug — the consumer's
throttled retry log is the operator-facing signal.
This file contains hidden or 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
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.
Scope
This PR bundles two related resilience themes for the 1.1.2 line, both touching the produce/fetch
paths and both needed for a consumer group to survive a broker restart:
acksoption reaches the broker again, andacks: 0fire-and-forget works.Each is described below. The commit history keeps them separable.
1. Produce: honour the acks option, make acks=0 fire-and-forget work
The bug
KafkaEx.API.produce/5documents a:required_acksoption. It never reached the wire.0.x translated it in the legacy adapter (
lib/kafka_ex/new/adapter.ex,acks: produce_request.required_acks). The 1.0 rewrite (573296b) removed that adapter and the translation went with it, while the public docs kept advertising the option. The request builder reads a different key,:acks, so since 1.0 every produce has gone out withacks: -1regardless of what the caller asked for.Two more defects sat behind that one, unreachable only because the first hid them:
acks: 0crashed the client —byte_size/1over the:okreturned by the async send path.{{:ok, response}, state}shape the retry layer expects.What changed
Option resolution. Resolved once in
KafkaEx.Client, order:acks>:required_acks>-1, feeding both the wire and the produce telemetry so the two can no longer disagree.:acksis now the canonical spelling;:required_acksis a deprecated alias, honoured until 2.0.Validation. Anything other than
-1/0/1is rejected with{:error, :invalid_acks}. Unvalidated it reached Kayrock's int16 encoder, whereacks: :all(the Java spelling) raised insidehandle_calland killed the client, andacks: 65_536truncated to0on the wire while the client waited for a response that would never come.Fire-and-forget.
acks: 0now takes a dedicated path that skips the retry layer — a resend would duplicate records, since the broker sends nothing to confirm the first one — and returns{:ok, %RecordMetadata{base_offset: nil}}.base_offsetwidens tonon_neg_integer() | nil. Java uses-1+hasOffset()and brod an unknown-offset marker;nilis used here because this struct has nohas_offset?/1andnilis already what the telemetry code treats as "no offset".Two failure modes fire-and-forget makes routine:
send_sync_request/3always did. Atacks: 0this is the only feedback channel there is, and discarding it left the next produce reusing a dead socket. (The close event is emitted once, by the client's active-mode error handler — see §2.)KafkaEx.Clientgains{:tcp_error, _, _}/{:ssl_error, _, _}clauses and a catch-allhandle_info/2. Defining anyhandle_info/2clause replaces the oneuse GenServerinjects, so an unmatched message crashed the client together with every broker socket and all cluster metadata — and a connection reset, which is exactly how a broker reports a rejectedacks: 0batch, arrives as{:tcp_error, _, :econnreset}ahead of{:tcp_closed, _}.Upgrade note
Callers who pass neither option are unaffected. Callers who pass one move from the durability they were silently getting to the one they asked for:
-1-1required_acks: 1-1, all in-sync replicas1, leader onlyrequired_acks: 0-1, all in-sync replicas, real offset0, fire-and-forget,base_offset: nilPass
acks: -1explicitly to keep the 1.0/1.1 behaviour. Code doing arithmetic onbase_offsetafter anacks: 0produce will now raise.Also worth knowing at
acks: 0:{:ok, _}means only that the batch was handed to the socket, and the[:kafka_ex, :produce, :stop]event carries no:offsetkey — a telemetry handler written asmetadata.offsetrather thanMap.get/2will raise and be detached.2. Consumer: survive leader moves and broker restarts
The bug
With replication-factor 1 (the AutoMQ default), restarting the broker that leads a partition killed the entire consumer group. The chain:
RF=1 makes the leaderless window wide (no follower to promote), but the same path runs on any leader move.
What changed
:leader_not_available,:not_leader_for_partition,:not_leader_or_follower,:kafka_storage_error,:replica_not_available) are retained with the leader the broker reported;select_node/2answers{:error, :leader_not_available}rather than resolving a negative node id to:broker_not_found. This also closes a silent per-key ordering bug: the default partitioner derives the partition count frommap_size(partition_leaders), so dropping a leaderless partition remapped keys.Retry.fetch_retryable?/1accepts (a leader move,:no_broker, a closed socket, a timeout) now backs off and retries — unbounded, matching brod, KafkaJS and librdkafka, none of which fail a consumer over a leaderless partition. Jittered exponential backoff (:fetch_retry_base_delay_ms500 →:fetch_retry_max_delay_ms5000). The backoff is held as a deadline in state, not a bare GenServer timeout, so an inbound call during a backoff can't cut the wait short. A failed:offset_out_of_rangereset rides the same backoff instead of raising aMatchError.{:tls_alert, _}tuple) was normalised to:unknown, which the fetch loop treated as fatal — so on TLS clusters the very broker-restart case above still killed the group. Such reasons now normalise to a retryable:transport_error.:fetch_unavailable_warn_ms(default 30000) of continuous failure, as aLogger.errorand a[:kafka_ex, :consumer, :partition_unavailable]telemetry event. Mirrors librdkafka'stopic.metadata.propagation.max.ms. The consumer keeps retrying.:no_brokersays why — unknown topic vs. leaderless partition vs. unknown node — logged (at debug; the consumer's throttled retry log is the operator-facing signal). The returned error is unchanged.acks: 0: the network layer no longer emits its own:send_errorclose, leaving the active-mode{:tcp_error}handler the single owner of[:kafka_ex, :connection, :close].What the code review changed (post-review commits)
An adversarial review of the first cut found, and this PR fixes: the
:transport_errorSSL hole (would have reopened the exact bug on TLS clusters); the duplicate close telemetry; and the amplified:no_brokerlog. It also confirmed, from the brod / Apache Kafka Java / librdkafka sources, thatunknown_topic_or_partitionis retried unbounded by all three — so no retry-count cap was added; the 30s time-window signal is the librdkafka-style precedent instead.Left out on purpose (deferred to 1.2.0)
Returning a concrete error instead of
:no_broker(needs anUPGRADING.mdentry, not patch/minor material);MetadataLog.missing_topics/2counting partitions rather than topic keys, and topic-level leaderless retention coupled with it;leader_epochstaleness checking;:kafka_storage_errorinfetch_retryable?/1.Tests
describe "handle_call/3 - produce acks"anddescribe "handle_info/2 - socket errors and unknown messages"inclient_test.exs, driven through the realhandle_call/3/handle_info/2withNetworkClientstubbed — both spellings and precedence, the effective default, telemetry parity, rejection of unusable values, the fire-and-forget round trip and its error path, and state threading (acorrelation_idassertion guarding against a dropped reconnected socket).reliability_test.exsnow asserts the realacks: 0behaviour and sends two async batches before a synchronous request on the same socket, proving the response-less produce leaves nothing to desynchronise framing.gen_consumer_fetch_retry_test.exsdrives the real consume loop throughKafkaEx.API.fetch/5— retries a transient error, retries a non-atom SSL transport error, still stops on a non-retryable error, recovers when the leader returns, backs off on a failed offset reset, serves a call during backoff, and surfacespartition_unavailableonce. Plusno_broker_reason_test.exs, the metadata-retention tests, and aretry.exoverflow regression (the old:math.powbackoff raisedArithmeticErrorpast attempt ~1023 — reachable only once retries became unbounded).Left out of automated runs
Mapping
:all/"all"to-1Java-style (a feature, not a fix), renaming therequired_ackstelemetry metadata field toacks(a documented contract change),:transactional_idbeing accepted but inert, andkayrock.md:149still documenting the 0.xKafkaEx.produce/4call removed in 1.0.Verification
mix test.unitmix format --check-formattedmix compile --warnings-as-errorsmix dialyzermix credo --strictCredo.Check.Design.DuplicatedCodeon Elixir 1.19; scoped checks on the changed files are clean. Needs CI.mix test.integration🤖 Generated with Claude Code