fix(inkless): Ensure additional system topics created as classic when log.diskless.enable=true [POD-1312]#586
fix(inkless): Ensure additional system topics created as classic when log.diskless.enable=true [POD-1312]#586
Conversation
There was a problem hiding this comment.
Pull request overview
Extends the “internal topic must remain classic (non-diskless)” guard to also cover additional system topics (__remote_log_metadata, __cluster_metadata) so they don’t inherit the broker default log.diskless.enable=true behavior.
Changes:
- Added a
ReplicationControlManager.isSystemTopic(...)helper and used it to prevent diskless topic creation (and diskless config reporting) for extra system topics. - Added controller tests to verify system topics are created as classic when server diskless is enabled, and that explicit
diskless.enable=trueis rejected for these topics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Introduces “system topic” detection and applies it to diskless enablement/rejection logic and config response handling. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds regression tests for classic creation and rejection of explicit diskless enablement on additional system topics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR ensures certain Kafka system topics (not covered by Topic.isInternal) are still created as classic (non-diskless) even when log.diskless.enable=true, and that attempts to explicitly enable diskless for these topics are rejected.
Changes:
- Add
ReplicationControlManager.isSystemTopic()and use it to block diskless creation / force diskless=false for system topics. - Centralize “additional system topics” handling and reuse it from
ClassicTopicRemoteStorageForcePolicy. - Add/adjust unit tests to cover creation and validation behavior for
__remote_log_metadataand__cluster_metadata.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Introduces isSystemTopic + additional system-topic list; updates diskless validation and config response logic to treat system topics as non-diskless. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Reuses ReplicationControlManager.isSystemTopic to avoid duplicated system-topic lists when deciding remote-storage forcing behavior. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds parameterized tests for system topics staying classic under diskless defaults and rejecting explicit diskless enable; updates expected error message text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR ensures certain Kafka system topics (not covered by Topic.isInternal) are always treated as classic topics when the server default log.diskless.enable=true, preventing them from being created as diskless.
Changes:
- Introduces
ReplicationControlManager.isSystemTopic(...)to extend the “internal topic” guard to additional system topics. - Updates topic-creation logic to reject explicit
diskless.enable=truefor system topics and to force returneddiskless.enable=falsein responses. - Adds/updates controller tests covering classic-creation behavior and explicit diskless rejection for the additional system topics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds isSystemTopic and applies it to diskless validation and config response shaping. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Reuses ReplicationControlManager.isSystemTopic to avoid duplicated special-case topic lists. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds tests for the additional system topics and updates expected error messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
17cdd8a to
d8497df
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the controller’s “non-diskless topic” guard so that additional Kafka system topics (not covered by Topic.isInternal) are created as classic even when log.diskless.enable=true, and rejects attempts to explicitly enable diskless on those topics.
Changes:
- Add
ReplicationControlManager.isSystemTopic()(internal + additional system topics) and use it in diskless-validation/response paths. - Update ClassicTopic remote-storage forcing logic to reuse the centralized system-topic check.
- Add/adjust unit tests to cover creating these system topics under diskless-by-default and error-message expectations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds isSystemTopic + additional system topic set; applies it to diskless enablement validation and config response shaping. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Reuses ReplicationControlManager.isSystemTopic instead of maintaining a separate additional-topic list. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds parameterized tests for __remote_log_metadata and __cluster_metadata behavior and updates error-message assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9868111 to
39dda17
Compare
There was a problem hiding this comment.
Pull request overview
Ensures certain Kafka “system topics” (not covered by Topic.isInternal) are always created/kept as classic (non-diskless) topics, even when the broker default enables diskless topics, and prevents configuration changes that would make those topics diskless.
Changes:
- Introduces
ReplicationControlManager.isSystemTopic()(internal + additional system topics) and applies it to topic-creation diskless validation and config reporting. - Persists an explicit
diskless.enableConfigRecord when the broker default is diskless, recordingfalsefor system topics to prevent inherited defaults. - Adds an IncrementalAlterConfigs guard rejecting
diskless.enable=truefor system topics; deduplicates remote-storage policy logic to use the shared helper; updates tests and error message wording.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds system-topic detection, blocks diskless on system topics at creation, and persists explicit diskless.enable when broker default is diskless. |
| metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java | Rejects enabling diskless via IncrementalAlterConfigs for system topics. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Removes duplicated topic list and delegates system-topic detection to ReplicationControlManager.isSystemTopic(). |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds/updates parameterized tests for system-topic diskless behavior and updated error messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
39dda17 to
791379f
Compare
There was a problem hiding this comment.
Pull request overview
This PR ensures certain controller-managed system topics (not covered by Topic.isInternal) are always treated as classic (non-diskless), even when log.diskless.enable=true is enabled at the broker level, and prevents diskless from being enabled for these topics via topic creation or AlterConfigs.
Changes:
- Introduces
ReplicationControlManager.isSystemTopic()to extend the “internal topic” guardrails to additional system topics (__remote_log_metadata,__cluster_metadata). - Persists an explicit
diskless.enableConfigRecord when the broker default is diskless, ensuring system topics explicitly recorddiskless.enable=false. - Rejects attempts to enable
diskless.enable=truefor system topics via CreateTopics and AlterConfigs (including legacy AlterConfigs), and deduplicates remote-storage policy logic to use the new system-topic helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds system-topic detection, extends diskless guards, and persists explicit diskless.enable based on broker default and topic type. |
| metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java | Rejects AlterConfigs enabling diskless on system topics. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Delegates system-topic exclusion to ReplicationControlManager.isSystemTopic() to avoid duplicated topic lists. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds/updates parameterized tests covering system-topic classic creation and diskless rejection via CreateTopics and AlterConfigs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Ensures certain Kafka system topics (not covered by Topic.INTERNAL_TOPICS) are always treated as classic (non-diskless) even when the broker default enables diskless topics, and prevents diskless enablement through config alteration paths.
Changes:
- Introduces
ReplicationControlManager.isSystemTopic()to classify internal + additional system topics and applies it to diskless creation guards and response config normalization. - Persists an explicit
diskless.enableConfigRecord when the broker default is diskless, usingfalsefor system topics to avoid inheriting the broker default. - Rejects
diskless.enable=truevia incremental/legacy AlterConfigs for system topics and deduplicates remote-storage forcing logic to rely on the centralized system-topic check.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds parameterized tests covering system-topic creation/alter-config rejection behavior and updates expected error messages. |
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds system-topic classification, enforces classic-only behavior for system topics, and persists explicit diskless.enable when broker default is diskless. |
| metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java | Adds validation to reject enabling diskless via AlterConfigs for system topics. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Removes duplicated topic list and delegates system-topic detection to ReplicationControlManager.isSystemTopic(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Ensures Kafka controller treats additional “system topics” (__remote_log_metadata, __cluster_metadata) as classic (non-diskless) even when the broker-level default enables diskless topics, and prevents enabling diskless on these topics via AlterConfigs.
Changes:
- Extend system-topic detection and enforce
diskless.enable=falsebehavior for those topics during creation (including explicit ConfigRecord persistence when server default is diskless). - Reject attempts to enable
diskless.enable=trueon system topics via AlterConfigs (incremental + legacy). - Deduplicate classic remote storage force-policy logic to reuse the shared system-topic check.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds system-topic classification and enforces classic semantics + explicit diskless.enable persistence under diskless-by-default brokers. |
| metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java | Rejects enabling diskless on system topics through AlterConfigs. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Delegates system-topic detection to ReplicationControlManager.isSystemTopic() to avoid duplication. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds parameterized tests for system-topic creation/alter-config rejection; updates error-message expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… log.diskless.enable=true These topics (__remote_log_metadata, __cluster_metadata) were not in Topic.INTERNAL_TOPICS so they would inherit the server default diskless setting. Added isSystemTopic() check that extends the internal topic guard to cover additional system topics that must remain classic. Also persists an explicit diskless.enable=false ConfigRecord for system topics when the broker default is diskless, preventing DescribeConfigs and effective-config resolution from inheriting the broker default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e wording - ClassicTopicRemoteStorageForcePolicy now delegates to ReplicationControlManager.isSystemTopic() instead of maintaining its own ADDITIONAL_INTERNAL_TOPICS set. - Error message updated from "Internal topics" to "System topics" to accurately reflect the expanded scope. - Fixed misleading test comment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
System topics must never become diskless, even when diskless.allow.from.classic.enable=true. Added a guard in ConfigurationControlManager to reject the alter operation unconditionally for system topics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
791379f to
c6d3cf8
Compare
There was a problem hiding this comment.
Pull request overview
Ensures certain controller-managed system topics (not covered by Topic.isInternal) are always created/treated as classic (non-diskless) even when the broker default enables diskless topics, and prevents enabling diskless on these topics via config APIs.
Changes:
- Introduces
ReplicationControlManager.isSystemTopic()and uses it to block diskless topic creation / effective config exposure for system topics. - Persists an explicit
diskless.enableConfigRecordwhen the broker default is diskless, writingfalsefor system topics to avoid inheriting the broker default. - Rejects
diskless.enable=truevia IncrementalAlterConfigs and legacy AlterConfigs for system topics, and deduplicates remote-storage force policy logic to use the shared system-topic predicate.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java | Adds system-topic detection and applies it to diskless creation guards, config-record persistence, and create-topics config reporting. |
| metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java | Rejects altering diskless.enable=true on existing system topics. |
| metadata/src/main/java/org/apache/kafka/controller/ClassicTopicRemoteStorageForcePolicy.java | Reuses the centralized system-topic predicate instead of maintaining a local special-case set. |
| metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java | Adds/updates parameterized coverage for system-topic creation behavior and AlterConfigs rejection paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
These topics (
__remote_log_metadata,__cluster_metadata) were not inTopic.INTERNAL_TOPICSso they would inherit the server default diskless setting. This PR ensures they are always treated as classic (non-diskless) topics.Changes
ReplicationControlManager.isSystemTopic()check that extends the internal topic guard to cover additional system topics that must remain classic.log.diskless.enable=trueat the broker level, system topics now get an explicitdiskless.enable=falseConfigRecord persisted, preventingDescribeConfigsand effective-config resolution from inheriting the broker default.diskless.enable=truevia AlterConfigs for system topics, even whendiskless.allow.from.classic.enable=true.ClassicTopicRemoteStorageForcePolicynow delegates toReplicationControlManager.isSystemTopic()instead of maintaining its ownADDITIONAL_INTERNAL_TOPICSset.Test plan
diskless.enable=truerejected on system topic creationdiskless.enable=truerejected for system topics (even with allow-from-classic)diskless.enable=falseConfigRecord