feat(inkless): add controller guards for classic-to-diskless switch [POD-2464] by EelisK · Pull Request #634 · aiven/inkless

EelisK · 2026-06-05T13:34:30Z

Enforce the following guards when altering the topic config to use diskless.enable=true:

Partitions are online
No reassignment in progress
Enough ISRs

Copilot

Pull request overview

This PR adds controller-side safety checks to prevent switching an existing (“classic”) topic to diskless (diskless.enable=true) unless key replication/availability preconditions are satisfied, and adds tests to validate rejection behavior.

Changes:

Add controller validation for classic-to-diskless transitions (online partitions, no reassignment in progress, healthy ISR/ELR state).
Apply the validation in the controller’s incrementalAlterConfigs handling path.
Add unit/integration tests asserting that unhealthy partitions cause the switch to be rejected.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java	Adds classic→diskless precondition validation and refactors topic filtering for switch-start record emission.
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java	Invokes the new precondition validation before applying incremental config changes.
metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java	Adds unit tests for precondition failures and pending-switch interactions.
core/src/test/java/kafka/server/InklessTopicTypeSwitcherClusterTest.java	Adds an integration test ensuring under-replicated topics cannot be switched to diskless via incremental alter configs.

Comments suppressed due to low confidence (1)

metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:1967

configChanges is mutated via configChanges.keySet().removeAll(...). Callers can (and do) pass immutable maps (e.g., Map.of(...) in QuorumControllerTest), which will throw UnsupportedOperationException even when preconditionErrors is empty. Avoid mutating the input map and instead operate on a mutable copy / filtered map for the subsequent config write and migration record generation.

            Map<ConfigResource, ApiError> preconditionErrors =
                    replicationControl.validateClassicToDisklessSwitchPreconditions(configChanges);
            configChanges.keySet().removeAll(preconditionErrors.keySet());

            ControllerResult<Map<ConfigResource, ApiError>> result =
                configurationControl.incrementalAlterConfigs(configChanges, false);
            result.response().putAll(preconditionErrors);
            if (validateOnly) {
                return result.withoutRecords();
            } else {
                List<ApiMessageAndVersion> migrationRecords =
                    replicationControl.markClassicToDisklessSwitchStarted(configChanges, result.response());
                if (!migrationRecords.isEmpty()) {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

giuseppelillo · 2026-06-09T07:59:26Z

+            Map<ConfigResource, ApiError> preconditionErrors =
+                    replicationControl.validateClassicToDisklessSwitchPreconditions(configChanges);
+            configChanges.keySet().removeAll(preconditionErrors.keySet());
+
            ControllerResult<Map<ConfigResource, ApiError>> result =
                configurationControl.incrementalAlterConfigs(configChanges, false);
+            result.response().putAll(preconditionErrors);


Even though it logically makes sense to first validate and then apply incrementalAlterConfigs logic, it would break the error messages for some particular cases, like this one:
e.g. diskless.allow.from.classic.enable=false and the topic is under-replicated => this code will classify the topic as "enabling diskless”" return the under-replicated precondition error, and skip ConfigurationControlManager, which would have returned “It is invalid to enable diskless on an already existing topic.”

So the correct order here would be:

Run config validation first (configurationControl.incrementalAlterConfigs)

Only for resources whose config change is otherwise valid, run the replication-state preconditions

If preconditions fail, do not emit that resource’s ConfigRecord.

Thanks for the explanation 👍 addressed

Enforce the following guards when altering the topic config to use diskless.enable=true: - Partitions are online - No reassignment in progress - Enough ISRs [POD-2464]

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

…ing diskless switch

viktorsomogyi

LGTM on my part but let's wait for Giuseppe's verdict as he requested some changes earlier.

giuseppelillo

Overall LGTM, I like the approach.

However I think there's a latent bug in the Legacy AlterConfigs (which is present also in the already existing markClassicToDisklessSwitchStartedForLegacyAlterConfigs):
Legacy AlterConfigs deletes existing configs that are missing from the request. If a classic topic has explicit diskless.enable=false and the broker default is log.diskless.enable=true, a legacy request that only changes another config and omits diskless.enable can effectively switch the topic to diskless, but it won't be caught.

Could you add a test about this and verify that is a real bug, and if so fix it?

The legacy AlterConfigs API will implicitly switch to diskless in the following conditions: - Broker level config `log.diskless.enable` is `true` - The topic currently has `diskless.enable` set to `false` - The altered topic config does not include `diskless.enable` This commit enforces the same partition validations for those implicitly diskless topics as for explicit ones.

EelisK · 2026-06-11T15:00:11Z

@giuseppelillo good catch, it was indeed an issue 👍 I added the fix and tests e92e01c

…POD-2464] (#634) * feat(inkless): add controller guards for classic-to-diskless switch Enforce the following guards when altering the topic config to use diskless.enable=true: - Partitions are online - No reassignment in progress - Enough ISRs [POD-2464] * fix(inkless): block unclean election via electLeaders API during pending diskless switch * fix(inkless): detect implicit diskless switch in legacy AlterConfigs The legacy AlterConfigs API will implicitly switch to diskless in the following conditions: - Broker level config `log.diskless.enable` is `true` - The topic currently has `diskless.enable` set to `false` - The altered topic config does not include `diskless.enable` This commit enforces the same partition validations for those implicitly diskless topics as for explicit ones.

EelisK requested a review from Copilot June 8, 2026 06:49

Copilot started reviewing on behalf of EelisK June 8, 2026 06:49 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/QuorumController.java Outdated

Comment thread metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java

EelisK force-pushed the EelisK/POD-2464-partition-checks branch from 6738471 to f086555 Compare June 8, 2026 09:04

EelisK marked this pull request as ready for review June 8, 2026 10:16

giuseppelillo mentioned this pull request Jun 9, 2026

docs(inkless:switch): Document classic to diskless switch #632

Merged

giuseppelillo requested changes Jun 9, 2026

View reviewed changes

EelisK force-pushed the EelisK/POD-2464-partition-checks branch 2 times, most recently from 82898dd to 513d861 Compare June 10, 2026 13:29

feat(inkless): add controller guards for classic-to-diskless switch

3bf9745

Enforce the following guards when altering the topic config to use diskless.enable=true: - Partitions are online - No reassignment in progress - Enough ISRs [POD-2464]

EelisK force-pushed the EelisK/POD-2464-partition-checks branch from 513d861 to 3bf9745 Compare June 10, 2026 13:38

EelisK requested a review from Copilot June 10, 2026 13:39

Copilot started reviewing on behalf of EelisK June 10, 2026 13:40 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java

viktorsomogyi reviewed Jun 10, 2026

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java

viktorsomogyi reviewed Jun 10, 2026

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java

fix(inkless): block unclean election via electLeaders API during pend…

b7f8570

…ing diskless switch

EelisK requested a review from giuseppelillo June 11, 2026 07:55

viktorsomogyi previously approved these changes Jun 11, 2026

View reviewed changes

giuseppelillo requested changes Jun 11, 2026

View reviewed changes

EelisK dismissed viktorsomogyi’s stale review via e92e01c June 11, 2026 14:58

EelisK requested a review from giuseppelillo June 11, 2026 15:03

giuseppelillo approved these changes Jun 12, 2026

View reviewed changes

giuseppelillo merged commit 7830390 into main Jun 12, 2026
5 checks passed

giuseppelillo deleted the EelisK/POD-2464-partition-checks branch June 12, 2026 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(inkless): add controller guards for classic-to-diskless switch [POD-2464]#634

feat(inkless): add controller guards for classic-to-diskless switch [POD-2464]#634
giuseppelillo merged 3 commits into
mainfrom
EelisK/POD-2464-partition-checks

EelisK commented Jun 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseppelillo Jun 9, 2026

Uh oh!

EelisK Jun 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

viktorsomogyi left a comment

Uh oh!

giuseppelillo left a comment

Uh oh!

EelisK commented Jun 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

EelisK commented Jun 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseppelillo Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

EelisK Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

viktorsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

giuseppelillo left a comment

Choose a reason for hiding this comment

Uh oh!

EelisK commented Jun 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants