feat: Added support for all Kafka API older versions. - #338
Conversation
a498dd9 to
ae8281d
Compare
|
Reviewed the src side of this branch and pushed fixes in 08b2d49. Full local suite is green except the GSSAPI tests (no KDC image locally) and two schema registry tests that fail identically on the unmodified branch. Behaviour change worth calling out for the release notes. Code comparing against Related: the They are now translated to the wire values before being sent, with a test covering it. Also fixed
Not fixed, tracked instead Pinning ApiVersions to v1 costs Left alone The three |
61c3eff to
2d47391
Compare
2d47391 to
db5a843
Compare
Signed-off-by: Paolo Insogna <paolo@cowtech.it>
The lag tests read committed offsets before autocommit had covered the partitions they assert on, so getLag returned -1n instead of the expected lag. Wait for the relevant partitions to have a committed offset first. Also disable matrix fail-fast: a single flaky job was cancelling the other 23, which made one failing test look like a fully broken matrix. Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
Both tests used retry budgets too tight to survive CI load. send-when-another-destination-fails assumed exactly two Produce requests and that the non-mocked one always succeeds. The producer can issue a third (repeatOnStaleMetadata defaults to true), and under load the non-mocked destination hits a genuine retriable broker error. With retries: 0 both destinations failed and produced.offsets came back empty. The mocked error is canRetry: false, so that destination still fails permanently and the assertion keeps its original meaning. gh-300 failed its initial refresh with "listOffsets failed 2 times" when a single ListOffsets was slow. The wider budget does not affect the pause/resume window the test exercises. Verified on a 24-job matrix: the producer test failed 6 jobs and gh-300 failed 4 before this change, and neither failed any job after it. Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
Consumer group states are now reported as Kafka writes them on the wire. listGroups and describeGroups upper cased the raw value, which turned PreparingRebalance into PREPARINGREBALANCE: a value present in neither ConsumerGroupStates nor KafkaConsumerGroupStates, yet cast to ConsumerGroupStateValue. The Pascal case constants this branch added are now the values actually returned. listGroups also translates the legacy Java enum constant names before sending them. Brokers match the states filter case insensitively but not underscore insensitively, so PREPARING_REBALANCE passed client side validation and then silently matched nothing on the broker. Other fixes: - allowedConfigSources is deduplicated, since DYNAMIC_TOPIC_CONFIG and its TOPIC_CONFIG alias share the value 1 and a JSON Schema enum requires unique items. - appendUUID rejects values which do not serialize to 16 bytes. UUIDs carry no length prefix and Buffer.from() stops at the first non hex character, so a topic name used as a topic id appended nothing at all and desynced the rest of the request. - appendVarIntBytes skips empty buffers, like appendString and appendBytes already do. Appending one corrupts DynamicBuffer's positional reads, and record keys and values are allowed to be empty without being null. Reported as platformatic/dynamic-buffer#12. - The Fetch v12 name to id map is built once per request instead of once per request and once per response, keeping the allocation out of the fetch loop's callback. - Produce v3 to v6 no longer set writer.context.requestTimeout: nothing reads it, the connection uses its own requestTimeout option, and v7 to v11 never set it. - The two admin fan outs aggregate throttleTimeMs the same way, and by reducing rather than spreading a caller sized array. - The ApiVersions v1 pinning documents what it costs, tracked in #343. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
The protocol tests parse bytes the same test authored with Writer, so a misread schema is wrong in both directions and still passes. Nothing in this branch had been validated by a broker, and the CI matrix could not do it either: Base[kGetApi] always walks down from maxVersion, so against every supported broker the newest codec wins and the 148 legacy ones are never serialized, sent or parsed. pinApiVersions rewrites the negotiated range on a real client, which needs no production change, and forEachVersion sweeps an API across every version the broker still accepts. Versions below a broker's floor are reported as diagnostics rather than quietly skipped. No old broker images are needed: Confluent 7.5.0 still advertises a minimum of v0 for nearly every API, so 141 of the 148 new codecs are reachable on brokers already in the matrix. This found two real bugs: - DeleteTopics v4 and v5 sent a tagged field section after each topic name, which made the broker drop the connection. topic_names is an array of plain COMPACT_STRINGs at those versions and only becomes an array of structs in v6. The protocol tests asserted the same wrong bytes, so they are corrected too. A sweep of every other flexible codec found no second instance. - listGroups reported an empty state below ListGroups v4, where group_state does not exist on the wire. It now reports 'Unknown', which is the value Kafka defines for the case and was already in ConsumerGroupStates. Run them with `npm run test:compat`. The `.compat-test.ts` suffix keeps them out of `npm test`, following the convention the memory tests already use. The CI job which runs them is not in this commit: pushing a workflow change needs a token scope this machine does not have, so it has to be added separately. Delegation tokens need a broker secret key to be reachable at all, so broker-sasl now sets one. The codecs which remain unreachable, AlterPartition v0-v3 and delegation token v0, are documented in the API status page rather than left to look covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
The main matrix cannot reach the legacy codecs: the client always negotiates the newest version a broker advertises, so only pinning exposes them. This job runs the sweeps added in 287cfd9. Both broker versions are needed and neither is redundant. Kafka 4.0 raised the minimum accepted version of several APIs (KIP-896), so 7.5.0 reaches the oldest codecs (Fetch v0-v3, CreateTopics v0-v1, OffsetCommit v0-v1) while 8.2.0 catches anything which assumes an old broker. One Node.js version is enough because these exercise wire formats rather than runtime behaviour, so this adds two jobs rather than twenty four. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
287cfd9 set KAFKA_DELEGATION_TOKEN_SECRET_KEY on broker-sasl so the delegation token codecs could be exercised. That broke all three Confluent 7.5.0 jobs: KRaft only gained delegation token support in Apache Kafka 3.6 (KIP-900), so a 3.5 broker configured with a secret key refuses to start outright. java.lang.UnsupportedOperationException: Delegation tokens are not supported The broker exited before any test ran, which is why the failure was Node.js version independent and confined to 7.5.0. The setting moves to docker-compose.delegation-tokens.yml, applied explicitly and only where the feature exists. The sweeps skip themselves with a diagnostic on brokers which report the APIs as UNSUPPORTED, which is what 7.5.0 does, so they pass either way. Verified both ways: broker-sasl starts clean on 7.5.0 with the base compose, and the sweeps stay at 272 of 272 on 8.2.0 with the override applied. The compatibility job still needs to pass the override file for 8.2.0. That is a workflow change, which needs a token scope this machine does not have, so it follows separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
The compatibility job failed on Confluent 7.5.0. The broker reports a minimum of
v0 for OffsetCommit and OffsetFetch, so the sweeps exercised those versions, and
the broker answered every one of them with UNSUPPORTED_VERSION:
✖ OffsetCommit v0 + OffsetFetch v0
Error: The version of API is not supported.
Those versions stored consumer offsets in ZooKeeper, which KRaft does not have,
so a KRaft broker refuses them while still advertising them. That is a property
of the broker rather than a defect in the codec, and the same mismatch can appear
for any API on any broker, so it is now detected generically: runAtVersion turns a
protocol level UNSUPPORTED_VERSION into a reported skip, and leaves every other
error to fail as before.
Verified against both ends of the matrix: 252 of 252 on Confluent 7.5.0 and 272 of
272 on 8.2.0. The 7.5.0 run reports exactly what it could not reach, which is the
point of the harness:
ℹ Produce: skipping v10, v11 — not accepted by this broker
ℹ OffsetCommit v0 + OffsetFetch v0: the broker advertises this version but rejects it, skipping
ℹ This broker does not support the delegation token APIs at all, skipping.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
1.1.0 is the oldest broker this package claims to support, and until now nothing verified that claim. It is the only broker where the legacy codecs are what the client actually negotiates rather than what a test pins, and the only one where ApiVersions maxes out at v1, which is the reason base.ts pins v1 in the first place. docker-compose.legacy.yml is a standalone stack rather than an override of docker-compose.yml: pre-KRaft brokers need ZooKeeper and must not receive the KAFKA_PROCESS_ROLES, CLUSTER_ID and KAFKA_CONTROLLER_* settings, and a compose override can add environment keys but never remove them. Each logical cluster gets its own ZooKeeper chroot, without which all five brokers join a single Kafka cluster and a client bootstrapping on one receives metadata for listeners it cannot speak. The sweeps pass 99 of 99 there. Three things had to change to get that far: - The compatibility helper creates topics with an explicit partition count and replication factor. Omitting them makes Admin send -1, meaning "use the broker default", which is KIP-464 and only understood from Apache Kafka 2.4. Older brokers answer INVALID_PARTITIONS or INVALID_REPLICATION_FACTOR. - OffsetCommit and OffsetFetch v0 read and write offsets in ZooKeeper while v1 and above use the group coordinator, so mixed pairs cannot round-trip on a broker old enough to still accept v0. Those combinations are no longer generated. - SASL and the delegation tokens which depend on it are skipped through COMPAT_LEGACY_BROKER, because Connection pins SaslAuthenticate to v2 and that version only exists from Apache Kafka 2.4. This is a real limitation rather than a missing broker feature, reported as #350, and the documented SASL floor is corrected from 1.0 to 2.4 to match reality. The flag is an explicit opt out rather than a catch of connection failures, so a genuine SASL regression on a modern broker still fails. Verified on both stacks: 99 of 99 on Apache Kafka 1.1.0 and 272 of 272 on Confluent 8.2.0, which is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
Connection used saslAuthenticateV2 unconditionally. v1 arrived in Apache Kafka 2.0
and v2 in 2.4, so on anything older the broker could not parse the request and
dropped the connection, which surfaced as a bare connection failure:
InvalidRequestException: Error getting request for apiKey: SASL_AUTHENTICATE,
apiVersion: 2, listenerName: ListenerName(SASL)
SASL therefore never worked below Kafka 2.4, even though everything else in the
client works on 1.1.0 and the documentation claimed 1.0.
The connection now asks the broker what it supports and picks the newest version
it implements within that range. Brokers accept ApiVersions before authentication
precisely so clients can ask, and the probe has to run before SaslHandshake because
once the handshake is sent the connection only accepts SaslAuthenticate. The result
is cached per connection, since reauthentication takes the same path. When a broker
does not answer the probe the previous behaviour is kept, so a connection which
worked before still works.
SaslHandshake stays pinned to v1 on purpose: v0 selects the pre-1.0 flow, where
SASL tokens are written raw on the wire rather than wrapped in SaslAuthenticate
requests, which this package does not implement.
Fixes #350.
The compatibility sweeps now run in full against Apache Kafka 1.1.0, 106 of 106,
with no opt out, so the COMPAT_LEGACY_BROKER flag added alongside the legacy stack
is gone. That broker exercises SaslAuthenticate v0, which is what a regression to a
pinned version would break, and it is also the only one which reaches the
delegation token v0 codecs: every later broker advertises a minimum of v1, so those
four are no longer listed as unreachable.
Verified on Confluent 8.2.0 as well: 2530 of 2531 in the main suite and 272 of 272
in the sweeps, both unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA
* perf: load test every implemented protocol version, fix readUUID The compatibility branch made Base[kGetApi] able to negotiate down to Produce v3 and Fetch v4, but every broker in CI negotiates to the newest version, so the legacy codecs were covered for correctness and never for speed. This adds the harness to measure them and records the results. The experiment pins the client codec against a fixed modern broker, so the protocol version is the only variable. Running against the 1.1.0 stack instead would vary the JVM, the storage engine and the codec at once, so that runs as a labelled sanity check rather than a measurement. Verdict: the legacy codecs are not slower. Across 9 Produce versions, 14 Fetch versions, three payload shapes and two acks settings, no legacy version is consistently slower than the newest. Where a reproducible difference exists it runs the other way. Two findings, both in the newest versions: - Reader.readUUID hyphenated with a capture group regex, 801ns against 56ns for the equivalent slicing. Fetch v13+ identifies topics by UUID, so this made the newest Fetch versions 15-18% slower to decode than the ones using topic names. Fixed here; 1653 protocol tests unchanged. - Flexible framing costs ~23% per message at 1MB fetch responses, with the break exactly at the v11/v12 boundary, reproduced over four runs. The mechanism is not isolated and is recorded rather than guessed at. LOAD_TESTING.md also records what the measurement cannot support: tier 1 Fetch at maxBytes=4096 and tier 0 Fetch decode are too noisy on this box for the 15% threshold, and the methodology errors found along the way - fixed record timestamps triggering broker retention mid-sweep, and a constant shuffle seed confounding version with position - both of which first presented as findings about the client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA * docs: add a README for the protocol version benchmarks Covers how to run the suite, what each script measures, and the configuration. The larger half is how to get numbers that mean something: vary the shuffle seed before believing any single-version result, pin CPUs, run one sweep at a time, and know the noise floor. Each of those countermeasures exists because its absence produced a plausible finding that turned out to be an artifact, so the README says which artifact and how it presented rather than just stating the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA * docs: remove LOAD_TESTING.md, fold its results into the suite README The plan document has served its purpose now that the suite exists and has run. Its two durable halves move into benchmarks/protocol-versions/README.md: the recorded verdict and the findings, which would otherwise survive only in the PR description, and the tier and guard naming that the source comments refer to. Nine references across seven files pointed at the deleted path; all now point at the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj49eYmNCELDfrP2xa1YpA --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
db5a843 to
226f30b
Compare
Signed-off-by: Paolo Insogna paolo@cowtech.it
Fixes #343.