Make Internal Compacted Topic Partition Count & Replication Factor Configurable#1933
Make Internal Compacted Topic Partition Count & Replication Factor Configurable#1933senthuran16 wants to merge 9 commits into
Conversation
…tition Egw/kafka sub topic partition
📝 WalkthroughWalkthroughThis pull request introduces configurable Kafka compacted topic partition and replication factor settings. The global Kafka runtime configuration gains two integer fields with default value 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
event-gateway/gateway-runtime/internal/connectors/brokerdriver/kafka/config_test.go (1)
154-191: 💤 Low valueConsider expanding test coverage for
intOverride.The current tests validate non-integer, out-of-bounds, and NaN
float64inputs. Consider adding test cases for:
- Positive/negative infinity:
math.Inf(1)andmath.Inf(-1)- Direct
inttype input (line 305 inconfig.gohandles this case)nilinput- Invalid types like
stringThese additions would provide more comprehensive coverage of the helper's type handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@event-gateway/gateway-runtime/internal/connectors/brokerdriver/kafka/config_test.go` around lines 154 - 191, Expand TestIntOverride_RejectsInvalidFloat64 (or add a new table-driven test) to cover more input types for intOverride: add cases for math.Inf(1) and math.Inf(-1) expecting rejection with "non-finite", add a case passing an int (e.g. 42) that should return ok==true and no error, add a nil input expecting error and ok==false, and add an invalid type like a string expecting error and ok==false; use the same pattern of t.Run and assertions (check err nil/nonnull, ok flag, and error message contents) so the test exercises intOverride's type handling branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go`:
- Around line 221-224: The consumerGroupID function now truncates the SHA256
hash to 32 chars instead of 16, which changes group IDs and will orphan existing
WebSub consumers; revert or migrate: update consumerGroupID (function
consumerGroupID in ConsumerManager) to preserve the previous format (use
hex.EncodeToString(h[:])[:16]) or implement compatibility by generating the old
16-char ID when a matching consumer group exists (or support both IDs for a
transition period), and add a clear release-note/migration entry describing the
breaking change and steps to migrate offsets if you choose the 32-char format.
---
Nitpick comments:
In
`@event-gateway/gateway-runtime/internal/connectors/brokerdriver/kafka/config_test.go`:
- Around line 154-191: Expand TestIntOverride_RejectsInvalidFloat64 (or add a
new table-driven test) to cover more input types for intOverride: add cases for
math.Inf(1) and math.Inf(-1) expecting rejection with "non-finite", add a case
passing an int (e.g. 42) that should return ok==true and no error, add a nil
input expecting error and ok==false, and add an invalid type like a string
expecting error and ok==false; use the same pattern of t.Run and assertions
(check err nil/nonnull, ok flag, and error message contents) so the test
exercises intOverride's type handling branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f04cb9d3-f051-4665-bb28-7170537092ff
📒 Files selected for processing (6)
event-gateway/gateway-runtime/configs/config.tomlevent-gateway/gateway-runtime/internal/config/config.goevent-gateway/gateway-runtime/internal/connectors/brokerdriver/kafka/config.goevent-gateway/gateway-runtime/internal/connectors/brokerdriver/kafka/config_test.goevent-gateway/gateway-runtime/internal/connectors/brokerdriver/kafka/endpoint.goevent-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go
|
Holding merging this PR until the test suite is ready |
Purpose
Internal compacted Kafka topics were always created with
1partition and replication factor1, which ignores cluster policy and deployment-specific sizing needs.Resolves #1882
Goals
Approach
kafka.compact_topic_partitionsandkafka.compact_topic_replication_factorto the existing runtime Kafka config pathCreateTopics(..., 1, 1, ...)call inEnsureCompactedTopic(...)with the resolved config valuesUser stories
Operators can align internal compacted topic creation with cluster replication and partitioning requirements without patching code.
Documentation
N/A - config-only runtime behavior change; no product doc update included in this PR.
Automation tests
Security checks
Samples
N/A
Related PRs
This PR replaces Handle WebSub delta updates without full undeploy/redeploy #1921
Handle WebSub delta updates without full undeploy/redeploy #1918
Egw/kafka sub topic partition #1932
Test environment
Ubuntu 24.04.4 LTS, Go package tests run locally with
GOCACHE=/tmp/go-build-cache