Repository navigation
Stop produce loops for nonexistent partitions - #331
novatechflow wants to merge 3 commits into
Conversation
novatechflow
left a comment
There was a problem hiding this comment.
Reviewed 808db4c; no remaining blockers.
The original implementation reproduced the busy loop for acknowledgements -1, 1, and 0, including requests with no timeout. It also returned the wrong error when auto-creation was disabled. The fix bounds each partition-log lookup to one auto-creation attempt and one retry, then reports UNKNOWN_TOPIC_OR_PARTITION if the partition is still absent. Requests with acks=0 complete without a response.
Hard review also reproduced the wrong error when a second broker encountered a lease for the nonexistent partition. The fix refreshes metadata once after failed ownership checks; real non-owner partitions retain leader redirection, stale metadata is refreshed, and unavailable metadata does not invent an unknown-partition error.
Ten repeated race-detector runs pass for invalid and boundary partition IDs, mixed valid/invalid requests, concurrent topic creation, metadata remaining stale after creation, normal auto-creation, embedded-etcd leases across two brokers, and 32 concurrent producers with unique acknowledged offsets. Existing partition counts and valid offsets are preserved, no log is initialized for the rejected partition, and valid partitions in the same request still succeed. Full repository race tests, vet, hosted CI, and CodeQL pass on this head.
Release-note draft updated: producing to a nonexistent partition no longer spins or waits for the request timeout; requests requiring a response receive the correct partition error, while acks=0 retains its no-response behavior. No configuration migration is required.
PaxMachinaOne
left a comment
There was a problem hiding this comment.
🤖 PaxMachina automated review (worker-code)
The loop fix is sound, but lease-error classification mishandles invalid topic names.
cmd/broker/main.go:1065:knownis initialized before confirming a successful topic metadata entry. For an invalid topic,MetadatareturnsINVALID_TOPIC_EXCEPTION; the loop skips that entry, leavesknownempty, and converts another broker’sErrNotOwnerintoErrUnknownTopic. Thus identical invalid requests can returnINVALID_TOPIC_EXCEPTIONfrom the lease owner butUNKNOWN_TOPIC_OR_PARTITIONfrom another broker. Preserve/map the topic-level error instead of treating every nonzero metadata error as proof that the partition is absent.
novatechflow
left a comment
There was a problem hiding this comment.
Confirmed the review finding. It was valid and is fixed in ca429d6.
Topic metadata errors are stored separately from partition membership. Only successful topic entries build a partition set; nonzero topic errors retain their Kafka error codes. Invalid names now return INVALID_TOPIC_EXCEPTION from both lease owners and other brokers.
Reproduced the original failure for all six invalid names in the existing test set, plus injected authorization, leader, timeout, and server metadata errors. Ten repeated race-detector runs pass with two broker handlers sharing embedded etcd, alongside the unknown-partition, ownership, stale-metadata, and concurrent auto-creation regressions. Full repository race tests, vet, hosted CI, and CodeQL pass on this head.
Re-review found no remaining blockers. The release-note draft now also notes that topic-level metadata errors are preserved across lease owners.
PaxMachinaOne
left a comment
There was a problem hiding this comment.
🤖 PaxMachina automated review (worker-code)
The retry cap is sound, but missing-topic auto-creation still permits a partition-ID resource-exhaustion path.
- High:
loadPartitionLogpasses the client-controlled partition ID toensureTopic(cmd/broker/main.go:2329), which calculatesNumPartitions = partition + 1(cmd/broker/main.go:2220). For a missing topic, a request targeting partition2147483646attempts to allocate roughly 2.1 billion partition metadata entries, potentially crashing the broker;MaxInt32also overflowspartition+1and produces incorrect behavior. The boundary test only uses an existing topic, so it never exercises this auto-creation path. Validate/cap requested partition IDs before auto-creation and add a missing-topic boundary regression.
Summary
Fixes #326. Producing to a partition absent from an existing topic could repeatedly auto-create the topic, receive
ErrTopicExists, and retry the same failed lookup forever. Allow one auto-creation attempt per partition-log lookup, then returnUNKNOWN_TOPIC_OR_PARTITIONif the partition is still missing. Requests withacks=0complete without sending a response.A lease left by a prior invalid request also caused a second broker to return the wrong ownership error. Failed ownership checks now refresh metadata once per request and distinguish a nonexistent partition from a real partition owned elsewhere. Healthy owned requests skip this work. A failed metadata refresh does not infer that a partition is absent. Topic-level metadata errors are preserved, including
INVALID_TOPIC_EXCEPTION, instead of being misclassified as a missing partition. Partition membership is checked only for successful topic entries.Valid partitions in the same request still succeed. Missing-topic auto-creation continues to work, while concurrent creation or stale metadata cannot cause another unbounded retry. The unreleased notes are updated.
Validation
go test -race -p 1 ./...andgo vet ./...pass onca429d6. Hosted CI and CodeQL also pass, including authentication coverage and all end-to-end checks.