fix(inkless:controller): validate remote storage on consolidated diskless creates by giuseppelillo · Pull Request #845 · aiven/inkless

giuseppelillo · 2026-10-29T14:03:01Z

With consolidation on, the controller turns remote storage on for every new diskless topic that doesn't set it. It added the remote.storage.enable=true record after it validated the create, so LogConfig skipped the tiered-topic checks. A diskless topic created with a retention below the broker log.local.retention.ms was accepted, and every later AlterConfigs on it failed with INVALID_CONFIG.

The controller now adds remote.storage.enable=true to the create request after the create-topic interceptors and before validation, so an invalid create fails at create time. An explicit value is kept, so the existing check still rejects false. The consolidation guide no longer describes the old behavior.

Test plan

Add tests in ReplicationControlManagerInklessTest that check the validated configs include the flag for explicit, default, and forced diskless creates, and that a validation failure rejects the create. They fail without this change.
Extend InklessUntieredDisklessMigrationTest with the reproduction from the ticket.
Run ReplicationControlManagerInklessTest, ReplicationControlManagerTest, and checkstyle.
Run make vale.

Assisted-by: Claude Opus 5.5

Copilot

🟢 Approval recommended

No active review issues remain for the consolidated diskless topic validation change.

No open issues 🎉

What changed in this PR

This PR fixes create-time validation for consolidated diskless topics. The controller adds the implicit remote.storage.enable=true setting before validation, preventing invalid retention settings from passing creation and blocking later configuration changes.

Changes:

Add the remote-storage default after create-topic interceptors and before validation, preserving explicit values.
Add unit and integration coverage for validation, rejected creates, and valid retention overrides.
Update the consolidation guide to describe create-time rejection.

File	Description
`metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java`	Support injecting a configuration validator into the test context.
`metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerInklessTest.java`	Test remote-storage validation for explicit, default, and forced diskless creates.
`metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java`	Add implicit remote storage to diskless creation configs before validation.
`docs/inkless/DISKLESS_CONSOLIDATION.md`	Document create-time rejection of incompatible retention settings.
`core/src/test/java/kafka/server/InklessUntieredDisklessMigrationTest.java`	Cover short-retention rejection, a valid override, and a subsequent configuration change.

_{Review effort: Balanced}

…less creates [KC-657] With consolidation on, the controller turns remote storage on for every new diskless topic that doesn't set it. It added the `remote.storage.enable=true` record after it validated the create, so LogConfig skipped the tiered-topic checks. A diskless topic created with a retention below the broker `log.local.retention.ms` was accepted, and every later AlterConfigs on it failed with `INVALID_CONFIG`. The controller now adds `remote.storage.enable=true` to the create request after the create-topic interceptors and before validation, so an invalid create fails at create time and `incrementalAlterConfig` emits the config record. The post-validation record in `validConfigRecords()` is gone, so the flag has a single injection point. An explicit value is kept, so the existing check still rejects `false`. The consolidation guide no longer describes the old behavior. ## Test plan - Add tests in `ReplicationControlManagerInklessTest` that check the validated configs include the flag for explicit, default, and forced diskless creates, and that a validation failure rejects the create. They fail without this change. - Extend `InklessUntieredDisklessMigrationTest` with the reproduction from the ticket. - Run `ReplicationControlManagerInklessTest`, `ReplicationControlManagerTest`, `QuorumController*` tests, and checkstyle. - Run `make vale`. Assisted-by: Claude Opus 5.5 Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

🟢 Approval recommended

No active review issues were identified for the create-time validation change for consolidated diskless topics.

No open issues 🎉

_{Review effort: Balanced}

viktorsomogyi · 2026-11-02T11:05:03Z

+    /**
+     * Adds {@code remote.storage.enable=true} to the config ops of a new diskless topic when consolidation
+     * is on, so that LogConfig validation checks the topic as a tiered topic. Without the flag in the
+     * validated request, a create could store a local retention larger than the topic retention, and every
+     * later AlterConfigs on the topic would fail.
+     *
+     * <p>Runs after the create-topic interceptors, which can set {@code diskless.enable}. An explicit
+     * {@code remote.storage.enable} is kept, so the later checks reject a {@code false} value.
+     */


Not sure if we need this comment on the private method given that the method itself is fairly simple.

The method is short. The comment is there for the part the body does not show: this runs after the create-topic interceptors and before incrementalAlterConfig, and putIfAbsent leaves an explicit remote.storage.enable=false so createTopic can reject it.

I will cut it down to that.

Pushed a new commit for this
E870

viktorsomogyi · 2026-11-02T11:40:16Z

+        Map<String, Entry<OpType, String>> keyToOps
+    ) {
+        if (!isDisklessRemoteStorageConsolidationEnabled || isSystemTopic(topicName)) return;
+        Entry<OpType, String> disklessOp = keyToOps.get(DISKLESS_ENABLE_CONFIG);


So when we use log.diskless.enable and omit diskless.enable on topic creation, then it won't be part of keyToOps and the fact that diskless is enabled is materialized after validateConfigs(). In this case it may be possible to validate a topic as a tiered topic instead of a CDT. Perhaps we could materialize log.diskless.enable before validation to be completely symmetric.

The order is what you describe. A create that omits diskless.enable leaves that key out of keyToOps, and validConfigRecords writes diskless.enable=true afterward from defaultDisklessEnable.

maybeAddRemoteStorageEnableForConsolidation still inserts remote.storage.enable=true on that path, because a missing op falls back to defaultDisklessEnable. LogConfig therefore runs the tiered retention checks. testValidationSeesAutoEnabledRemoteStorage covers explicit, default, and forced diskless.

LogConfig does not treat that create as consolidating. willConsolidateDisklessTopic() requires diskless.enable in the merged topic config, and the validator ignores log.diskless.enable on purpose: an existing tiered topic stays non-consolidating when the broker default is on.

The consolidating checks that depend on the broker default run in createTopic, which does see it:

remote.log.copy.disable=true fails there (testDefaultDisklessEnableRejectsRemoteLogCopyDisable).

An explicit remote.storage.enable=false fails there.

Putting diskless.enable=true into keyToOps before validation would make the validator see a consolidating topic. With consolidation on, isDisklessConsolidationModeOnCreation() allows both flags, so the accept or reject result stays the same. I would keep the persisted default in validConfigRecords, which also forces false for system topics.

…C-657] The method body already shows what it adds. The comment keeps the ordering the body does not show: after the create-topic interceptors and before validation, with an explicit false left for createTopic to reject. Assisted-by: Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

…less creates (#845) * fix(inkless:controller): validate remote storage on consolidated diskless creates [KC-657] With consolidation on, the controller turns remote storage on for every new diskless topic that doesn't set it. It added the `remote.storage.enable=true` record after it validated the create, so LogConfig skipped the tiered-topic checks. A diskless topic created with a retention below the broker `log.local.retention.ms` was accepted, and every later AlterConfigs on it failed with `INVALID_CONFIG`. The controller now adds `remote.storage.enable=true` to the create request after the create-topic interceptors and before validation, so an invalid create fails at create time and `incrementalAlterConfig` emits the config record. The post-validation record in `validConfigRecords()` is gone, so the flag has a single injection point. An explicit value is kept, so the existing check still rejects `false`. The consolidation guide no longer describes the old behavior. ## Test plan - Add tests in `ReplicationControlManagerInklessTest` that check the validated configs include the flag for explicit, default, and forced diskless creates, and that a validation failure rejects the create. They fail without this change. - Extend `InklessUntieredDisklessMigrationTest` with the reproduction from the ticket. - Run `ReplicationC 8000 ontrolManagerInklessTest`, `ReplicationControlManagerTest`, `QuorumController*` tests, and checkstyle. - Run `make vale`. Assisted-by: Claude Opus 5.5 Co-authored-by: Cursor <cursoragent@cursor.com> * docs(inkless:controller): shorten the consolidation create comment [KC-657] The method body already shows what it adds. The comment keeps the ordering the body does not show: after the create-topic interceptors and before validation, with an explicit false left for createTopic to reject. Assisted-by: Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>

On 4.3, `incrementalAlterConfig`, `legacyAlterConfigs`, `ReplicationControlManager#createTopics`, `Controller#createTopics`, and `ControllerApis#createTopics` take a `forwarded` flag, and `MetadataDelta` has no public constructor, only `MetadataDelta.Builder`. The tests added by #832, #835, #839, #845, #846, and #852 used the main signatures. Assisted-by: Claude Opus 5.5

…less creates (#845) * fix(inkless:controller): validate remote storage on consolidated diskless creates [KC-657] With consolidation on, the controller turns remote storage on for every new diskless topic that doesn't set it. It added the `remote.storage.enable=true` record after it validated the create, so LogConfig skipped the tiered-topic checks. A diskless topic created with a retention below the broker `log.local.retention.ms` was accepted, and every later AlterConfigs on it failed with `INVALID_CONFIG`. The controller now adds `remote.storage.enable=true` to the create request after the create-topic interceptors and before validation, so an invalid create fails at create time and `incrementalAlterConfig` emits the config record. The post-validation record in `validConfigRecords()` is gone, so the flag has a single injection point. An explicit value is kept, so the existing check still rejects `false`. The consolidation guide no longer describes the old behavior. ## Test plan - Add tests in `ReplicationControlManagerInklessTest` that check the validated configs include the flag for explicit, default, and forced diskless creates, and that a validation failure rejects the create. They fail without this change. - Extend `InklessUntieredDisklessMigrationTest` with the reproduction from the ticket. - Run `ReplicationControlManagerInklessTest`, `ReplicationControlManagerTest`, `QuorumController*` tests, and checkstyle. - Run `make vale`. Assisted-by: Claude Opus 5.5 Co-authored-by: Cursor <cursoragent@cursor.com> * docs(inkless:controller): shorten the consolidation create comment [KC-657] The method body already shows what it adds. The comment keeps the ordering the body does not show: after the create-topic interceptors and before validation, with an explicit false left for createTopic to reject. Assisted-by: Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>

giuseppelillo requested a balanced review from Copilot October 29, 2026 14:07

Copilot started reviewing on behalf of giuseppelillo October 29, 2026 14:08

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

giuseppelillo force-pushed the cursor/validate-consolidated-diskless-create branch from dd4cb40 to 82316a7 Compare October 29, 2026 14:15

giuseppelillo requested a balanced review from Copilot October 29, 2026 14:18

Copilot started reviewing on behalf of giuseppelillo October 29, 2026 14:18

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

giuseppelillo marked this pull request as ready for review October 29, 2026 14:22

viktorsomogyi reviewed Nov 2, 2026

View reviewed changes

viktorsomogyi approved these changes Nov 2, 2026

View reviewed changes

viktorsomogyi merged commit c8bb204 into main Nov 2, 2026
7 checks passed

viktorsomogyi deleted the cursor/validate-consolidated-diskless-create branch November 2, 2026 15:32

Navigation Menu

Repository navigation

fix(inkless:controller): validate remote storage on consolidated diskless creates - #845

fix(inkless:controller): validate remote storage on consolidated diskless creates#845
viktorsomogyi merged 2 commits into
mainfrom
cursor/validate-consolidated-diskless-create

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Test plan

Uh oh!

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants