Repository navigation
fix(kafka): build the producer at the first publish, not at connect - #223
Conversation
## Summary `KafkaClient::connect` built an idempotent producer for every client, and librdkafka requests a producer id 500 ms after creation. A broker authorizes that as `IdempotentWrite` on the cluster or `Write` on a topic, which a service that only consumes lacks. - The producer is now built at the first publish, on a blocking thread, and shared by every clone. - `ping` uses one metadata consumer, built at the first probe under the probe's timeout, so a health check builds no producer. - `shutdown` waits for every build in flight, a cancelled publish's included, and refuses a later one. - A configuration librdkafka refuses is reported by the first client built. ## Tests - New `kafka_consume_only_mock`: `a_client_that_only_consumes_sends_no_init_producer_id`, red before the fix with one `InitProducerId`, and `a_client_closed_before_any_publish_builds_no_producer`. - Seven new `client::tests`: `connect_builds_no_producer_and_concurrent_clones_share_one`, `a_publish_after_shutdown_builds_no_producer`, `rejected_tls_material_fails_the_first_publish_and_leaves_the_cell_empty`, `a_producer_build_blocked_in_librdkafka_leaves_the_runtime_free`, `shutdown_waits_for_a_producer_build_in_flight`, `a_probe_times_out_while_its_metadata_consumer_is_built`, `shutdown_waits_for_a_build_whose_publish_was_cancelled`. ## Test plan - `cargo nextest run --features <kafka set>` on the mock files and the Docker-backed `kafka_ping` (27 passed), `--lib` (641 passed), `--no-default-features` (379 passed) - The five Docker-backed Kafka suites (109 passed) - `cargo fmt -- --check` and `cargo clippy -D warnings` on every CI feature set, the nine `--lib` sets included
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
zannis
left a comment
There was a problem hiding this comment.
Building the producer at the first publish is the right fix, and the mock-cluster test that counts InitProducerId after the 500 ms timer is a good guard. I think the change can be smaller: build the ping client at connect (which also keeps config errors failing at startup), and drop the builds_in_flight lock, which OnceCell already makes unnecessary. Details inline, plus one docs wording fix.
| /// broker dedupe by producer id + sequence, restoring at-least-once into | ||
| /// exactly-once-on-the-broker semantics. Requires Kafka ≥ 0.11 | ||
| /// (universal today) and caps in-flight requests per connection at 5. | ||
| async fn producer(&self) -> Result<&KafkaProducerInner> { |
There was a problem hiding this comment.
With the producer built lazily, a missing write permission only shows up at a consumer's first republish. That includes a handler timeout or panic, both of which become Retry. A consumer that can publish (RetryStrategy::Republish, the default on owned topologies, or a declared DLQ) needs Write on the target topic, plus IdempotentWrite on brokers before 2.8. Without it, InitProducerId fails with CLUSTER_AUTHORIZATION_FAILED, librdkafka marks the producer fatal, and the cell keeps it for the client's lifetime. Then:
- Concurrent path: the partition's commits stop at the first retried record.
PerRecord: the member reconnects in a loop on that record.- Sequenced topics: the next commit moves past the retried message, so it's dropped and not counted.
- DLQ: every reject is lost.
Could the consumer check this at startup when its config can publish, and fail with an error naming the missing permission? Two options:
- Call
producer()at start, wait about 500 ms plus one round trip, and checkclient().fatal_error(). It's simple, but it's a negative-only signal. - Describe the topic and the DLQ with authorized operations included (KIP-430) and check for
WRITE. The answer is definite and needs onlyDescribe, but rdkafka 0.39 doesn't wrap it, so it would go throughrdkafka-sysFFI.
InPlace with no DLQ, and broadcast, would skip the check.
There was a problem hiding this comment.
Moving this to a follow-up PR: a startup check that a consumer able to publish (Republish retries or a DLQ) is allowed to write. Not blocking this one.
`connect` builds the consumer-type metadata client `ping` probes with, where it used to build the producer. A connection setting librdkafka refuses therefore fails at startup again, and a client keeps one idle librdkafka client as before. The probe in `ping` fetches on that client, so the lazily built metadata cell and its timeout case are gone. `shutdown` no longer holds a lock over producer builds. The failing initializer on the producer cell already waits for a build in flight. The cancelled token refuses every build that starts later, so the lock added nothing. The doc comment on the producer config states the idempotence rationale in two sentences. The comments on the lazy producer shrink to what the code does not say. The ACL paragraph in the Kafka backend page narrows "a service that only consumes" to a service that never publishes, through `publisher()` or a republish. Tests: `rejected_tls_material_fails_connect`, `a_probe_reports_a_refused_configuration` and `a_failed_producer_build_leaves_the_cell_empty_for_the_next_publish` cover the connect-time refusal and the producer build failure. Two tests cover the MSK IAM metadata client: `msk_iam_connect_rejects_tls_material_librdkafka_refuses` and `msk_iam_ping_times_out_against_a_non_routable_broker`. The tests `shutdown_waits_for_a_build_whose_publish_was_cancelled` and `a_probe_times_out_while_its_metadata_consumer_is_built` go with the code they tested.
zannis
left a comment
There was a problem hiding this comment.
The second commit covers the three review points: the metadata client is built at connect again, so refused TLS/SASL settings fail at startup; the build lock is gone; and the docs sentence is narrowed. The startup write-permission check moves to a follow-up. That hazard predates this change: on main the idempotent producer already went fatal shortly after connect.
Summary
KafkaClient::connectbuilt an idempotent producer for every client, and librdkafka requests a producer id 500 ms after creation. A broker authorizes that asIdempotentWriteon the cluster orWriteon a topic, which a service that only consumes lacks.The producer is now built at the first publish, on a blocking thread, and shared by every clone. Instead,
connectbuilds the consumer-type metadata client thatpingprobes with, so a connection setting librdkafka refuses still fails at startup. A client therefore keeps one idle librdkafka client, as before. The close inshutdownwaits for a producer build in flight and refuses a later one.Tests
kafka_consume_only_mock:a_client_that_only_consumes_sends_no_init_producer_id, red before the fix with oneInitProducerId, anda_client_closed_before_any_publish_builds_no_producer.client::tests:connect_builds_no_producer_and_concurrent_clones_share_one,a_publish_after_shutdown_builds_no_producer,a_failed_producer_build_leaves_the_cell_empty_for_the_next_publish,rejected_tls_material_fails_connect,a_probe_reports_a_refused_configuration,a_producer_build_blocked_in_librdkafka_leaves_the_runtime_free,shutdown_waits_for_a_producer_build_in_flight.kafka_msk_iam_integration:msk_iam_connect_rejects_tls_material_librdkafka_refuses,msk_iam_ping_times_out_against_a_non_routable_broker, andmsk_iam_generate_oauth_token_does_not_panicnow closes the client after its publish.Test plan
cargo nextest run --features <kafka set>on the mock, MSK IAM and Docker-backed Kafka suites,--lib, and--no-default-featurescargo fmt -- --checkandcargo clippy -D warningson every CI feature set, the nine--libsets included