feat(controller:diskless): add partitions support for diskless topics#540
feat(controller:diskless): add partitions support for diskless topics#540viktorsomogyi merged 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves CREATE_PARTITIONS behavior for diskless topics by aligning validation and test coverage with diskless “managed replicas” vs “legacy/unmanaged” modes in the controller.
Changes:
- Reject manual partition assignments when adding partitions to diskless topics while diskless managed replicas are disabled.
- Add unit tests covering add-partitions auto-placement, manual-assignment acceptance/rejection rules, and ISR filtering for fenced brokers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds diskless-specific validation to createPartitions to reject manual assignments when managed replicas are disabled. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds tests for diskless add-partitions behavior across unmanaged vs managed-replicas configurations, including ISR expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java
Show resolved
Hide resolved
metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java
Show resolved
Hide resolved
Add partitions now works correctly for diskless topics: - Auto-placement inherits RF from existing partitions and uses standard ReplicaPlacer (works for both managed and unmanaged) - Manual assignments are allowed for managed-RF diskless topics - Manual assignments are rejected for unmanaged diskless topics, matching the createTopics restriction - ISR for new partitions correctly filters to active brokers only
ffa6c82 to
9a37b24
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends diskless topic support in the controller to correctly handle partition additions and “immediate” reassignment semantics, including stricter validation for unmanaged diskless topics and correct ISR behavior based on broker liveness.
Changes:
- Update diskless partition reassignment to apply replica changes immediately and compute ISR from active brokers only, including rejecting assignments where all targets are fenced.
- Update leader balancing / preferred-leader imbalance tracking to skip diskless topics.
- Add controller tests covering diskless createPartitions auto-placement, manual-assignment validation (managed vs unmanaged), reassignment-to-fenced behavior, and leader balancing skip logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Implements diskless-specific behavior for reassignment/ISR and excludes diskless topics from preferred-leader imbalance tracking; adds createPartitions validation for unmanaged diskless topics. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds/updates diskless tests for immediate reassignment semantics, fenced-broker ISR filtering/rejection, and createPartitions behavior for managed/unmanaged diskless topics. |
Comments suppressed due to low confidence (1)
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:2634
- In the diskless reassignment branch, we set target replicas/ISR but never clear
addingReplicas/removingReplicas. If the partition currently has an in-progress reassignment (e.g., from an earlier controller version or a prior request), this path can leave reassignment state behind andlistPartitionReassignmentswill continue to report it as ongoing—contradicting the intended “complete immediately” behavior. Consider explicitly setting targetAdding/targetRemoving to empty whenisDisklessand applying the direct assignment (and/or unconditionally for diskless reassignments).
leaderAcceptor,
featureControl.metadataVersionOrThrow(),
getTopicEffectiveMinIsr(topics.get(tp.topicId()).name)
);
builder.setEligibleLeaderReplicasEnabled(featureControl.isElrFeatureEnabled());
if (isDiskless) {
// Diskless: data is in object storage, no replica sync needed.
// Apply target replicas directly — skip the staged adding/removing process
// (no addingReplicas/removingReplicas). This is safe because:
//
// 1. No offline risk: the reassignment is rejected if all target brokers are
// fenced (see activeIsr check below). As long as at least one target broker
// is active, PartitionChangeBuilder.electLeader() will elect it as leader
// since the leaderAcceptor above only requires isActive (no directory check).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…licas Currently adding partitions will run a replica placer which is not aligned with the logic on topic creation. Covering this properly to have separate logic between unmanaged and managed diskless replicas.
…dd-partitions support The config comment and doc string said "only affects topic creation" but add-partitions now also respects the managed replicas setting.
…#540) * feat(controller:diskless): add partitions support for diskless topics Add partitions now works correctly for diskless topics: - Auto-placement inherits RF from existing partitions and uses standard ReplicaPlacer (works for both managed and unmanaged) - Manual assignments are allowed for managed-RF diskless topics - Manual assignments are rejected for unmanaged diskless topics, matching the createTopics restriction - ISR for new partitions correctly filters to active brokers only * refactor(controller:diskless): handle add partition for unmanaged replicas Currently adding partitions will run a replica placer which is not aligned with the logic on topic creation. Covering this properly to have separate logic between unmanaged and managed diskless replicas. * fix(config:diskless): update managed replicas config doc to reflect add-partitions support The config comment and doc string said "only affects topic creation" but add-partitions now also respects the managed replicas setting. (cherry picked from commit e88df37)
…#540) * feat(controller:diskless): add partitions support for diskless topics Add partitions now works correctly for diskless topics: - Auto-placement inherits RF from existing partitions and uses standard ReplicaPlacer (works for both managed and unmanaged) - Manual assignments are allowed for managed-RF diskless topics - Manual assignments are rejected for unmanaged diskless topics, matching the createTopics restriction - ISR for new partitions correctly filters to active brokers only * refactor(controller:diskless): handle add partition for unmanaged replicas Currently adding partitions will run a replica placer which is not aligned with the logic on topic creation. Covering this properly to have separate logic between unmanaged and managed diskless replicas. * fix(config:diskless): update managed replicas config doc to reflect add-partitions support The config comment and doc string said "only affects topic creation" but add-partitions now also respects the managed replicas setting.
…#540) * feat(controller:diskless): add partitions support for diskless topics Add partitions now works correctly for diskless topics: - Auto-placement inherits RF from existing partitions and uses standard ReplicaPlacer (works for both managed and unmanaged) - Manual assignments are allowed for managed-RF diskless topics - Manual assignments are rejected for unmanaged diskless topics, matching the createTopics restriction - ISR for new partitions correctly filters to active brokers only * refactor(controller:diskless): handle add partition for unmanaged replicas Currently adding partitions will run a replica placer which is not aligned with the logic on topic creation. Covering this properly to have separate logic between unmanaged and managed diskless replicas. * fix(config:diskless): update managed replicas config doc to reflect add-partitions support The config comment and doc string said "only affects topic creation" but add-partitions now also respects the managed replicas setting. (cherry picked from commit e88df37)
Add partitions now works correctly for diskless topics:
Additionally, it updates how unmanaged replicas handle AddPartition calls, aligning it with CreateTopic logic; and updates server configs to align with current logic.