Repository navigation
feat(gateways): Kafka Gateway add DescribeConfigs and AlterConfigs APIs - #4398
Conversation
…n clients need to read and set topic retention. DescribeConfigs reports retention.ms from Iggy message expiry and a read-only cleanup.policy. AlterConfigs persists only retention.ms, capped so segment expiry cannot wrap.
…n clients need to read and set topic retention. DescribeConfigs reports retention.ms from Iggy message expiry and a read-only cleanup.policy.AlterConfigs persists only retention.ms, capped so segment expiry cannot wrap.
gateways/kafka/docs/CONFIGS.md gateways/kafka/src/protocol/handlers/alter_configs.rs gateways/kafka/src/protocol/handlers/describe_configs.rs gateways/kafka/src/protocol/handlers/topic_config.rs gateways/kafka/tests/common/wire.rs gateways/kafka/tests/configs_handler_tests.rs gateways/kafka/tests/configs_java_client_tests.rs gateways/kafka/tests/configs_real_bridge_tests.rs
|
Keeping it in draft mode as there few edge cases needs to be worked and documented |
Milliseconds stay the value stored in Iggy. Those names are remembered as synonyms only when an alter uses them.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4398 +/- ##
============================================
+ Coverage 88.43% 88.45% +0.02%
+ Complexity 1607 1579 -28
============================================
Files 1302 1293 -9
Lines 252565 248952 -3613
Branches 212730 212367 -363
============================================
- Hits 223358 220217 -3141
+ Misses 23811 23537 -274
+ Partials 5396 5198 -198
🚀 New features to boost your workflow:
|
|
/skill team-review-slim |
There was a problem hiding this comment.
Summary: The new Kafka DescribeConfigs and AlterConfigs handlers carry three client-visible defects: per-resource Iggy round trips that serialize on the shared lockstep client, an unmodeled describe key that fails the whole resource, and an empty AlterConfigs list answered as success. Two test comments are stale, and two simplifications are noted.
Counts: critical 0, warning 3, nit 2, simplification 2
This review was generated by Claude Code 2.1.284 on deepseek-flash[1m]. Review the output before you act on it.
|
/ready |
krishvishal
left a comment
There was a problem hiding this comment.
Main issues: the timeout comment in alter_configs.rs does not match the code, repeated describe keys blow up the response, and retention.minutes/hours are not Kafka topic configs.
Details inline. The simplification comments are cleanups, not blockers.
|
/ready |
Which issue does this PR address?
Partially closes #3546 (Part 2)
Relates to #3560
Rationale
Kafka admin clients read and change topic retention with DescribeConfigs (32) and the deprecated AlterConfigs (33). Iggy already stores that lifetime as
message_expiry, and retention applies to sealed segments only. The active segment does not expire. CreateTopics rejects a per-topic config list and tells the client to setretention.mswith AlterConfigs, so these two APIs are how a client reads and writes that expiry after the topic exists.This PR covers the topic-config half only. Unknown keys are
INVALID_CONFIG(40). Kafka has noNOT_CONFIGURABLEcode, which is the code the issue text suggested.What changed?
With the bridge on, DescribeConfigs (versions 1-4) answers topic resources only. It returns
retention.msfrom Iggymessage_expiry(-1when sealed segments never expire) and a read-onlycleanup.policy=delete. AlterConfigs (versions 0-2) persists onlyretention.ms, throughupdate_topic. A value whose seconds exceedu32::MAXis rejected and is not stored.validate_onlyruns the same checks, including the topic lookup, and does not write.One bad key fails that resource only. Other resources in the same request are still processed. With the bridge off, each resource gets
NOT_CONTROLLER(41) and nothing is read or written. A missing topic isUNKNOWN_TOPIC_OR_PARTITION(3), checked before the config keys. More than 100 distinct topics returnsPOLICY_VIOLATION(44) on every resource in the request.IncrementalAlterConfigs (44) is not implemented and is not advertised. A client that sends it gets the connection closed. Only the deprecated
Admin.alterConfigs(33) writesretention.ms. CreateTopics still rejects per-topic configs and tells the client to use AlterConfigs. DeleteTopics is not in this PR and is covered in a separate PR. IncrementalAlterConfigs (44) will be implemented in another succeding PRs.Local Execution
Passed:
cargo build -p iggy-gateway-kafka --testscargo clippy -p iggy-gateway-kafka --all-features --all-targets -- -D warningsretention.ms=-1, then alter to8000, unknown key rejected,validate_onlydid not write,describe_batch_50_elapsed_ms=22. The probe prints that elapsed time and does not assert a latency threshold.AI Usage
Fill this in before opening.