KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests #15719

brandboat · 2024-04-15T16:30:03Z

Introduce a new internal config log.initial.task.delay.ms to control InitialTaskDelayMs in LogManager to speed up tests

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…in LogManager to speed up tests

showuon

@brandboat , thanks for quick PR, overall LGTM! Left some comments.

showuon · 2024-04-16T12:13:34Z

core/src/main/scala/kafka/server/KafkaConfig.scala

@@ -835,6 +836,7 @@ object KafkaConfig {
      .define(CreateTopicPolicyClassNameProp, CLASS, null, LOW, CreateTopicPolicyClassNameDoc)
      .define(AlterConfigPolicyClassNameProp, CLASS, null, LOW, AlterConfigPolicyClassNameDoc)
      .define(LogMessageDownConversionEnableProp, BOOLEAN, LogConfig.DEFAULT_MESSAGE_DOWNCONVERSION_ENABLE, LOW, LogMessageDownConversionEnableDoc)
+      .defineInternal(LogInitialTaskDelayMsProp, LONG, LogConfig.DEFAULT_INITIAL_TASK_DELAY_MS, LOW)


Could we add a doc at the last parameter for other developers know what this config is doing for?
Ex:
The initial task delay in millisecond when initializing tasks in LogManager. This should be used for testing only.

Also, we can set atLeast(0) in the defineInternal method. So that we don't need additional validator below.

showuon · 2024-04-16T12:17:31Z

core/src/main/java/kafka/server/builders/LogManagerBuilder.java

@@ -179,6 +179,7 @@ public LogManager build() {
                              logDirFailureChannel,
                              time,
                              keepPartitionMetadataFile,
-                              remoteStorageSystemEnable);
+                              remoteStorageSystemEnable,
+                              LogConfig.DEFAULT_INITIAL_TASK_DELAY_MS);


I know currently we don't use LogManagerBuilder in the tests, but I still think we should add a initialTaskDelayMs setting and set default value to LogConfig.DEFAULT_INITIAL_TASK_DELAY_MS.

showuon · 2024-04-16T12:20:59Z

core/src/test/scala/unit/kafka/log/LogManagerTest.scala

-    time.sleep(logManager.InitialTaskDelayMs)
+    time.sleep(logManager.initialTaskDelayMs)
    assertEquals(6, log.numberOfSegments, "Now there should be exactly 6 segments")
    time.sleep(log.config.fileDeleteDelayMs + 1)


Could we create a test in LogManagerTest to verify the logManager will start these tasks after customized initialTaskDelayMs? Adding a simple test like what we see here, or maybe we can directly add verification inside these tests?

Could we create a test in LogManagerTest to verify the logManager will start these tasks after customized initialTaskDelayMs

Huge thanks ! I'm a little confused about the comment above, the test in LogManagerTest itself verify that tasks like log cleanup, flush logs are triggered after sleeping initialTaskDelayMs.

Huge thanks ! I'm a little confused about the comment above, the test in LogManagerTest itself verify that tasks like log cleanup, flush logs are triggered after sleeping initialTaskDelayMs.

Yes, and currently, all of them are setting initialTaskDelayMs=LogConfig.DEFAULT_INITIAL_TASK_DELAY_MS. I'm thinking we could have a test and set initialTaskDelayMs to, let's say, 1000, and we can verify the change is adopted by verifying if we sleep only 1000ms, the log cleanup will be triggered.

Thanks for the explanation !

or maybe we can directly add verification inside these tests?

I decided to follow the comment as you mentioned earlier, and updated the initialTaskDelayMs to 10s in LogManagerTests. Please take another look 😃

soarez · 2024-04-16T14:02:31Z

core/src/test/scala/unit/kafka/utils/TestUtils.scala

@@ -1524,7 +1524,8 @@ object TestUtils extends Logging {
                   logDirFailureChannel = new LogDirFailureChannel(logDirs.size),
                   keepPartitionMetadataFile = true,
                   interBrokerProtocolVersion = interBrokerProtocolVersion,
-                   remoteStorageSystemEnable = remoteStorageSystemEnable)
+                   remoteStorageSystemEnable = remoteStorageSystemEnable,
+                   initialTaskDelayMs = LogConfig.DEFAULT_INITIAL_TASK_DELAY_MS)


If the goal is to speed up tests, shouldn't we use a lower value here?

Thanks for your comment @soarez 😃 ! If I understand correctly TestUtils#createLogManager use MockTime and the clock would advance immediately after invoking the time#sleep method in the test, pointing to the corresponding sleep time thereafter. The goal in this pr is to introduce a new internal config to speed up for tests like e2e/integration tests which can't use MockTime as above.

I see. Thanks for clarifying

No problem, thanks for reviewing the pr !

chia7712

@brandboat thanks for offering this nice function. one small comment left. overall + 1

chia7712 · 2024-04-16T20:12:59Z

core/src/main/java/kafka/server/builders/LogManagerBuilder.java

@@ -55,6 +55,7 @@ public class LogManagerBuilder {
    private Time time = Time.SYSTEM;
    private boolean keepPartitionMetadataFile = true;
    private boolean remoteStorageSystemEnable = false;
+    private long initialTaskDelayMs = LogConfig.DEFAULT_INITIAL_TASK_DELAY_MS;


please add setter for it

showuon

LGTM!

soarez · 2024-04-18T09:52:18Z

Thanks for the changes.
You have some compilation errors:

[2024-04-17T15:27:16.786Z] > Task :jmh-benchmarks:compileJava
[2024-04-17T15:27:16.786Z] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-15719/jmh-benchmarks/src/main/java/org/apache/kafka/jmh/server/CheckpointBench.java:110: error: method createLogManager in class TestUtils cannot be applied to given types;
[2024-04-17T15:27:16.786Z]         this.logManager = TestUtils.createLogManager(JavaConverters.asScalaBuffer(files),
[2024-04-17T15:27:16.786Z]                                    ^
[2024-04-17T15:27:16.786Z]   required: Seq<File>,LogConfig,ConfigRepository,CleanerConfig,MockTime,MetadataVersion,int,boolean,Option<UnifiedLog>,boolean,long
[2024-04-17T15:27:16.786Z]   found: Buffer<File>,LogConfig,MockConfigRepository,CleanerConfig,MockTime,MetadataVersion,int,boolean,Option<Object>,boolean
[2024-04-17T15:27:16.786Z]   reason: actual and formal argument lists differ in length
[2024-04-17T15:27:16.786Z] 1 error

Can you address these?

brandboat · 2024-04-18T10:47:51Z

Thanks for the reminder, soarez ! Already fixed the error.

chia7712 · 2024-04-19T08:56:15Z

@brandboat this PR is great. However, I'd like to merge it after #15569. #15569 is a huge PR which refactor the KafkaConfig and LogConfig, and I try to alleviate the pain of fixing conflicts continually.

brandboat · 2024-04-19T08:58:01Z

@brandboat this PR is great. However, I'd like to merge it after #15569. #15569 is a huge PR which refactor the KafkaConfig and LogConfig, and I try to alleviate the pain of fixing conflicts continually.

No problem ! Thanks for the notification !

chia7712 · 2024-04-19T20:14:58Z

@brandboat please fix the conflicts, thanks!

KAFKA-16552: Create an internal config to control InitialTaskDelayMs …

37987bf

…in LogManager to speed up tests

brandboat marked this pull request as ready for review April 15, 2024 16:30

showuon reviewed Apr 16, 2024

View reviewed changes

FrankYang0529 mentioned this pull request Apr 16, 2024

KAFKA-16424: remove truncated logs after alter dir #15616

Merged

3 tasks

soarez reviewed Apr 16, 2024

View reviewed changes

brandboat added 2 commits April 16, 2024 22:36

Address comments

de7eecb

Merge remote-tracking branch 'origin/trunk' into KAFKA-16552

b41ab1a

chia7712 reviewed Apr 16, 2024

View reviewed changes

brandboat added 2 commits April 17, 2024 19:27

Add setInitialTaskDelayMs in LogManagerBuilder

97768f3

Make initialTaskDelayMs 10s in LogManagerTest

cdf4e06

showuon approved these changes Apr 18, 2024

View reviewed changes

Fix compile error

0cd7dcd

Merge remote-tracking branch 'origin/trunk' into KAFKA-16552

70c99bd

chia7712 merged commit ced79ee into apache:trunk Apr 20, 2024
1 check failed

brandboat deleted the KAFKA-16552 branch April 20, 2024 15:09

kamalcph mentioned this pull request Apr 22, 2024

MINOR: Reduce the time taken to execute the TieredStorage tests. #15780

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests #15719

KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests #15719

brandboat commented Apr 15, 2024

showuon left a comment

showuon Apr 16, 2024

showuon Apr 16, 2024

showuon Apr 16, 2024

showuon Apr 16, 2024

brandboat Apr 16, 2024

showuon Apr 16, 2024

brandboat Apr 17, 2024

soarez Apr 16, 2024

brandboat Apr 16, 2024 •

edited

soarez Apr 16, 2024

brandboat Apr 16, 2024

chia7712 left a comment

chia7712 Apr 16, 2024

showuon left a comment

soarez commented Apr 18, 2024

brandboat commented Apr 18, 2024 •

edited

chia7712 commented Apr 19, 2024

brandboat commented Apr 19, 2024

chia7712 commented Apr 19, 2024

KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests #15719

KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests #15719

Conversation

brandboat commented Apr 15, 2024

Committer Checklist (excluded from commit message)

showuon 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

brandboat Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

showuon left a comment

Choose a reason for hiding this comment

soarez commented Apr 18, 2024

brandboat commented Apr 18, 2024 • edited

chia7712 commented Apr 19, 2024

brandboat commented Apr 19, 2024

chia7712 commented Apr 19, 2024

brandboat Apr 16, 2024 •

edited

brandboat commented Apr 18, 2024 •

edited