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;
    }

m1a2st · 2025-04-11T09:37:29Z

Thanks for @junrao, @chia7712 comments, This PR is already for review, PTAL.

# Conflicts: # core/src/main/scala/kafka/log/LogCleaner.scala # storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java

github-actions · 2025-04-12T03:17:51Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

chia7712 · 2025-04-14T13:14:32Z

clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java

@@ -214,6 +214,9 @@ public class TopicConfig {
        "configuration. If message.timestamp.type=CreateTime, the message will be rejected if the difference in " +
        "timestamps exceeds this specified threshold. This configuration is ignored if message.timestamp.type=LogAppendTime.";

+    // visible for testing 
+    public static final String INTERNAL_SEGMENT_BYTES_CONFIG = "internal.segment.bytes";


TopicConfig is a public class, so could you please move it to LogConfig?

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.

junrao · 2025-04-14T22:53:50Z

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

@@ -186,12 +188,13 @@ public Optional<String> serverConfigName(String configName) {
            .define(ServerLogConfigs.CREATE_TOPIC_POLICY_CLASS_NAME_CONFIG, CLASS, null, LOW, ServerLogConfigs.CREATE_TOPIC_POLICY_CLASS_NAME_DOC)
            .define(ServerLogConfigs.ALTER_CONFIG_POLICY_CLASS_NAME_CONFIG, CLASS, null, LOW, ServerLogConfigs.ALTER_CONFIG_POLICY_CLASS_NAME_DOC)
            .define(ServerLogConfigs.LOG_DIR_FAILURE_TIMEOUT_MS_CONFIG, LONG, ServerLogConfigs.LOG_DIR_FAILURE_TIMEOUT_MS_DEFAULT, atLeast(1), LOW, ServerLogConfigs.LOG_DIR_FAILURE_TIMEOUT_MS_DOC)
-            .defineInternal(ServerLogConfigs.LOG_INITIAL_TASK_DELAY_MS_CONFIG, LONG, ServerLogConfigs.LOG_INITIAL_TASK_DELAY_MS_DEFAULT, atLeast(0), LOW, ServerLogConfigs.LOG_INITIAL_TASK_DELAY_MS_DOC);
+            .defineInternal(ServerLogConfigs.LOG_INITIAL_TASK_DELAY_MS_CONFIG, LONG, ServerLogConfigs.LOG_INITIAL_TASK_DELAY_MS_DEFAULT, atLeast(0), LOW, ServerLogConfigs.LOG_INITIAL_TASK_DELAY_MS_DOC)
+            .defineInternal(LogConfig.INTERNAL_SEGMENT_BYTES_CONFIG, INT, null, null, LOW, ServerLogConfigs.LOG_SEGMENT_BYTES_DOC);


Could we document that this config is for testing?

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; }

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 6 commits April 11, 2025 00:14

fix MetadataLog Test

e2adfbe

fix some test

3d8e122

fix fail tests

303ef39

Merge remote-tracking branch 'origin/KAFKA-19080' into KAFKA-19080

3f1b757

fix config def error

71dbb89

fix fail test

4c62f11

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 11, 2025

m1a2st added 4 commits April 11, 2025 19:53

Merge branch 'trunk' into KAFKA-19080

c847bbc

Merge branch 'trunk' into KAFKA-19080

ad36e75

# Conflicts: # core/src/main/scala/kafka/log/LogCleaner.scala # storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java

Update LogCleaner file

5e4fe46

change segmentSize modifier

9b9f469

github-actions bot added the needs-attention label Apr 12, 2025

chia7712 reviewed Apr 14, 2025

View reviewed changes

m1a2st added 4 commits April 14, 2025 21:16

Merge branch 'trunk' into KAFKA-19080

4e7a080

addressed by comments

8b7d366

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

m1a2st commented Apr 11, 2025

github-actions bot commented Apr 12, 2025

chia7712 Apr 14, 2025

chia7712 commented Apr 15, 2025

junrao left a comment

junrao Apr 14, 2025

junrao Apr 15, 2025

chia7712 Apr 15, 2025

junrao Apr 15, 2025

chia7712 Apr 16, 2025

junrao Apr 16, 2025

chia7712 Apr 17, 2025

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

Are you sure you want to change the base?

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

m1a2st commented Apr 11, 2025

github-actions bot commented Apr 12, 2025

Choose a reason for hiding this comment

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

m1a2st commented Apr 4, 2025 •

edited by github-actions bot

Loading

m1a2st commented Apr 4, 2025 •

edited

Loading