[WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level #19371

m1a2st · 2025-04-04T09:28:59Z

The main reason is that we forgot setting the
TopicConfig.SEGMENT_BYTES_CONFIG at least to 1024 * 1024, thus
addressed it, and add a test for it.

m1a2st · 2025-04-04T13:50:30Z

Hello @junrao, @chia7712
I have a question about this problem.
In KAFKA-16368, only the ServerLogConfigs value for log.segment.bytes was modified to 1 MB. However, the TopicConfig which in LogConfig default for segment.bytes remains 14 bytes. Many tests rely on this small segment.bytes value to generate a large number of segments.

I think there are two possible approaches to resolve this:

Add atLeast(1024 * 1024) validation in LogConfig, which would require fixing around 400 tests.
Add a validator in LogManager#updateTopicConfig to validate the change request at runtime.

junrao · 2025-04-09T00:08:49Z

@m1a2st : Perhaps we could somehow allow the tests to set a small segment.bytes.

chia7712 · 2025-04-09T16:58:34Z

Maybe we can add a internal config to allow tests to define small size?

This reverts commit f707465.

This reverts commit a0342ed.

This reverts commit 0f766c5.

chia7712 · 2025-04-10T12:27:34Z

for example:

   this.internalSegmentSize = getString(TopicConfig.INTERNAL_SEGMENT_BYTES_CONFIG);

   ...
   
    public long segmentSize() {
        if (internalSegmentSize != null) return Long.parseLong(internalSegmentSize);
        return segmentMs;
    }

chia7712 · 2025-04-15T16:38:03Z

@m1a2st could you please check the failed tests?

junrao

@m1a2st : Thanks for the PR. Left a couple of comments.

storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java

junrao · 2025-04-15T17:37:06Z

core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

+  }
+
+  // visible for testing
+  def internalApply(


Instead of having an internalApply, could we just use the existing apply and add INTERNAL_SEGMENT_BYTES_CONFIG to MetadataLogConfig?

+1 to move the internal config to MetadataLogConfig, and it would be better to wait #19465 extracting the metadata-related configs from other class to MetadataLogConfig

I just realized that metadata log uses a different approach to allow tests to use a smaller segment bytes than allowed in production. That approach defines the original segment byte config with a small minimal requirement, but adds METADATA_LOG_SEGMENT_MIN_BYTES_CONFIG to enforce the actual minimal requirement in production. This new config could be changed in tests to allow for smaller minimal bytes. The benefit of this approach is that it allows the existing config to be used directly to set a smaller value for tests. The downside is that the doc for min value is inaccurate and the validation is done through a customized logic.

It would be useful to pick the same strategy between metadata log and regular log. The metadata log approach seems slightly better since it's less intrusive. We could fix the inaccurate min value description for production somehow.

Excuse me, the strategy used by metadata log is to add a "internal" config (METADATA_LOG_SEGMENT_MIN_BYTES_CONFIG) to change the (metadata) segment size in testing, and that is what we want to address in this PR - we add a "internal" config for regular log, and so the test can use the "smaller" segment size.

I am just saying that we now have two different ways to achieve the same goal. In the metadata log approach, you set the desired value through the original config, which is segment.bytes. You then set an internal config to change the min constraint.

The approach in this PR is to set the desired value through a different internal config.

It would be useful to choose same approach for both the metadata log and the regular log.

Personally, I prefer the approach of adding "internal".xxx config as it provide better user experience for public configs, allowing users to see the "correct" min value. Additionally, we can remove the customized logic of validation.

In short, I suggest to add following changes to this PR.

remove METADATA_LOG_SEGMENT_MIN_BYTES_CONFIG

remove MetadataLogConfig#logSegmentMinBytes

add internal.metadata.log.segment.bytes

customize MetadataLogConfig#logSegmentBytes as following code

public int logSegmentBytes() { if (internalSogSegmentBytes != null) return internalSogSegmentBytes; return logSegmentBytes; }

Thanks for @chia7712, @junrao comments, addressed it :)

Hmm, why do we need internalApply()? MetadataLogConfig has both the product and internal segment bytes configs and we could just pass both into LogConfig in apply(), right?

# Conflicts: # core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

junrao

@m1a2st : Thanks for the updated PR. A few more comments.

junrao · 2025-04-21T16:55:34Z

raft/src/main/java/org/apache/kafka/raft/MetadataLogConfig.java

@@ -85,14 +82,13 @@ public class MetadataLogConfig {
            .define(METADATA_SNAPSHOT_MAX_INTERVAL_MS_CONFIG, LONG, METADATA_SNAPSHOT_MAX_INTERVAL_MS_DEFAULT, atLeast(0), HIGH, METADATA_SNAPSHOT_MAX_INTERVAL_MS_DOC)
            .define(METADATA_LOG_DIR_CONFIG, STRING, null, null, HIGH, METADATA_LOG_DIR_DOC)
            .define(METADATA_LOG_SEGMENT_BYTES_CONFIG, INT, METADATA_LOG_SEGMENT_BYTES_DEFAULT, atLeast(Records.LOG_OVERHEAD), HIGH, METADATA_LOG_SEGMENT_BYTES_DOC)
-            .defineInternal(METADATA_LOG_SEGMENT_MIN_BYTES_CONFIG, INT, METADATA_LOG_SEGMENT_MIN_BYTES_DEFAULT, atLeast(Records.LOG_OVERHEAD), HIGH, METADATA_LOG_SEGMENT_MIN_BYTES_DOC)


We need to set the constraint for METADATA_LOG_SEGMENT_BYTES_CONFIG to be at least 8MB.

Also, I thought the plan is to remove METADATA_LOG_SEGMENT_MIN_BYTES_CONFIG, but add sth like METADATA_INTERNAL_LOG_SEGMENT_BYTES_CONFIG to match the design in LogConfig?

@junrao I have discussed with @m1a2st offline, and he will update the PR tomorrow

junrao · 2025-04-21T16:58:59Z

core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

+    val metadataLog: KafkaMetadataLog = createMetadataLog(topicPartition, topicId, dataDir, time, scheduler, config, nodeId, defaultLogConfig)
+
+    // Print a warning if users have overridden the internal config
+    if (config.logSegmentBytes() != KafkaRaftClient.MAX_BATCH_SIZE_BYTES) {


Hmm, we don't need this if we add the constraint directly to METADATA_LOG_SEGMENT_BYTES_CONFIG, right?

junrao · 2025-04-21T17:04:20Z

raft/src/main/java/org/apache/kafka/raft/MetadataLogConfig.java

@@ -112,15 +108,15 @@ public class MetadataLogConfig {
     * @param deleteDelayMillis The amount of time to wait before deleting a file from the filesystem
     */
    public MetadataLogConfig(int logSegmentBytes,
-                             int logSegmentMinBytes,
+                             int internalLogSegmentMinBytes,


Hmm, this should be renamed to internalLogSegmentBytes since it's no longer the minimum, right?

junrao · 2025-04-21T17:10:34Z

core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

+  }
+
+  // visible for testing
+  def internalApply(


Hmm, why do we need internalApply()? MetadataLogConfig has both the product and internal segment bytes configs and we could just pass both into LogConfig in apply(), right?

chia7712 · 2025-04-26T15:44:42Z

core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala

@@ -601,7 +602,7 @@ class SaslSslAdminIntegrationTest extends BaseAdminIntegrationTest with SaslSetu
    assertNotEquals(Uuid.ZERO_UUID, createResult.topicId(topic1).get())
    assertEquals(topicIds(topic1), createResult.topicId(topic1).get())
    assertFutureThrows(classOf[TopicAuthorizationException], createResult.topicId(topic2))
-    
+


please revert unrelated change

chia7712 · 2025-04-26T15:48:28Z

core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala

@@ -567,7 +568,7 @@ class SaslSslAdminIntegrationTest extends BaseAdminIntegrationTest with SaslSetu
    client.createAcls(List(denyAcl).asJava, new CreateAclsOptions()).all().get()

    val topics = Seq(topic1, topic2)
-    val configsOverride = Map(TopicConfig.SEGMENT_BYTES_CONFIG -> "100000").asJava
+    val configsOverride = Map(LogConfig.INTERNAL_SEGMENT_BYTES_CONFIG -> "100000").asJava


this test case is used to verify the custom topic-level config, so we can increase the value to make test pass.

chia7712 · 2025-04-26T16:07:49Z

raft/src/main/java/org/apache/kafka/raft/MetadataLogConfig.java

    public static final String METADATA_LOG_SEGMENT_BYTES_CONFIG = "metadata.log.segment.bytes";
    public static final String METADATA_LOG_SEGMENT_BYTES_DOC = "The maximum size of a single metadata log file.";
    public static final int METADATA_LOG_SEGMENT_BYTES_DEFAULT = 1024 * 1024 * 1024;

+    public static final String INTERNAL_METADATA_LOG_SEGMENT_BYTES_CONFIG = "internal.metadata.log.segment.bytes";


Do we need another internal config? User can configure internal.segment.bytes if they want to create small segment, right?

fix the topic level segment.bytes error

ef72eee

github-actions bot added triage PRs from the community tools storage Pull requests that target the storage module small Small PRs labels Apr 4, 2025

use spotlessApply

36e59c8

m1a2st changed the title ~~KAFKA-19080 The constraint on segment.ms is not enforced at topic level~~ [WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level Apr 4, 2025

update test default value

7a29f51

github-actions bot added the core Kafka Broker label Apr 4, 2025

wip

1d66fb0

m1a2st added 5 commits April 9, 2025 20:22

Merge branch 'trunk' into KAFKA-19080

60c01e9

revert test change

9398bae

add new constructor for test

0f766c5

add escape annotation

a0342ed

fix some test

f707465

github-actions bot removed the small Small PRs label Apr 9, 2025

m1a2st added 3 commits April 10, 2025 19:02

Revert "fix some test"

768c8ed

This reverts commit f707465.

Revert "add escape annotation"

640ca6b

This reverts commit a0342ed.

Revert "add new constructor for test"

2bfd478

This reverts commit 0f766c5.

fix all test

51da598

github-actions bot added streams kraft KIP-932 Queues for Kafka clients labels Apr 10, 2025

m1a2st added 2 commits April 10, 2025 23:30

fix some test

8c39276

fix MetadataLog Test

eefa646

m1a2st added 2 commits April 14, 2025 21:29

addressed by comments

5bbdf1f

update the test

2a3db85

github-actions bot removed needs-attention triage PRs from the community labels Apr 15, 2025

m1a2st added 2 commits April 16, 2025 00:39

Merge branch 'trunk' into KAFKA-19080

919365a

remove unused import

a332072

junrao reviewed Apr 15, 2025

View reviewed changes

m1a2st added 5 commits April 20, 2025 15:12

Merge branch 'trunk' into KAFKA-19080

b774d92

# Conflicts: # core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

move other internal config

6876739

update KafkaMetadataLog apply flow

34a794f

fix compile error

c10c9cb

fix fail test

ea981e3

github-actions bot added the build Gradle build or GitHub Actions label Apr 21, 2025

m1a2st added 3 commits April 21, 2025 11:26

fix fail test

934ac37

revert unused change

77898ed

revert unused change

1756fee

junrao reviewed Apr 21, 2025

View reviewed changes

m1a2st added 4 commits April 22, 2025 23:15

Merge branch 'trunk' into KAFKA-19080

0bfec85

temp

1a3f737

Merge branch 'trunk' into KAFKA-19080

5761c62

Merge branch 'trunk' into KAFKA-19080

06131bf

chia7712 changed the title ~~KAFKA-19080 The constraint on segment.ms is not enforced at topic level~~ (WIP) KAFKA-19080 The constraint on segment.ms is not enforced at topic level Apr 24, 2025

m1a2st added 3 commits April 25, 2025 10:00

completed the feature

7483a5d

fix fail test

048e052

fix fail test

76bd287

m1a2st changed the title ~~(WIP) KAFKA-19080 The constraint on segment.ms is not enforced at topic level~~ KAFKA-19080 The constraint on segment.ms is not enforced at topic level Apr 25, 2025

chia7712 reviewed Apr 26, 2025

View reviewed changes

addressed by comments

ce644ae

m1a2st changed the title ~~KAFKA-19080 The constraint on segment.ms is not enforced at topic level~~ [WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level Apr 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level #19371

[WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level #19371

m1a2st commented Apr 4, 2025 •

edited by github-actions bot

Loading

m1a2st commented Apr 4, 2025 •

edited

Loading

junrao commented Apr 9, 2025

chia7712 commented Apr 9, 2025

chia7712 commented Apr 10, 2025

chia7712 commented Apr 15, 2025

junrao left a comment

junrao Apr 15, 2025

chia7712 Apr 15, 2025

junrao Apr 15, 2025

chia7712 Apr 16, 2025

junrao Apr 16, 2025

chia7712 Apr 17, 2025

m1a2st Apr 21, 2025

junrao Apr 21, 2025

junrao left a comment

junrao Apr 21, 2025

chia7712 Apr 23, 2025

junrao Apr 21, 2025

junrao Apr 21, 2025

junrao Apr 21, 2025

chia7712 Apr 26, 2025

chia7712 Apr 26, 2025

chia7712 Apr 26, 2025

[WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level #19371

Are you sure you want to change the base?

[WIP]KAFKA-19080 The constraint on segment.ms is not enforced at topic level #19371

Conversation

m1a2st commented Apr 4, 2025 • edited by github-actions bot Loading

m1a2st commented Apr 4, 2025 • edited Loading

junrao commented Apr 9, 2025

chia7712 commented Apr 9, 2025

chia7712 commented Apr 10, 2025

chia7712 commented Apr 15, 2025

junrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1a2st commented Apr 4, 2025 •

edited by github-actions bot

Loading

m1a2st commented Apr 4, 2025 •

edited

Loading