Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
chia7712 merged PR #15719: URL: https://github.com/apache/kafka/pull/15719 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
chia7712 commented on PR #15719: URL: https://github.com/apache/kafka/pull/15719#issuecomment-2067224991 @brandboat please fix the conflicts, thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
chia7712 commented on PR #15719: URL: https://github.com/apache/kafka/pull/15719#issuecomment-2066133702 @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on PR #15719: URL: https://github.com/apache/kafka/pull/15719#issuecomment-2066136873 > @brandboat this PR is great. However, I'd like to merge it after https://github.com/apache/kafka/pull/15569. https://github.com/apache/kafka/pull/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 ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on PR #15719: URL: https://github.com/apache/kafka/pull/15719#issuecomment-2063574931 Thanks for the reminder, soarez ! Already fix the error. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
soarez commented on PR #15719: URL: https://github.com/apache/kafka/pull/15719#issuecomment-2063477565 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,LogConfig,ConfigRepository,CleanerConfig,MockTime,MetadataVersion,int,boolean,Option,boolean,long [2024-04-17T15:27:16.786Z] found: Buffer,LogConfig,MockConfigRepository,CleanerConfig,MockTime,MetadataVersion,int,boolean,Option,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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1569022628 ## core/src/test/scala/unit/kafka/log/LogManagerTest.scala: ## @@ -413,7 +413,7 @@ class LogManagerTest { assertEquals(numMessages * setSize / segmentBytes, log.numberOfSegments, "Check we have the expected number of segments.") // this cleanup shouldn't find any expired segments but should delete some to reduce size -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) Review Comment: 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 :smiley: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
showuon commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1568045423 ## core/src/test/scala/unit/kafka/log/LogManagerTest.scala: ## @@ -413,7 +413,7 @@ class LogManagerTest { assertEquals(numMessages * setSize / segmentBytes, log.numberOfSegments, "Check we have the expected number of segments.") // this cleanup shouldn't find any expired segments but should delete some to reduce size -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) Review Comment: > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
chia7712 commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567891428 ## 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; Review Comment: please add setter for it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567499221 ## 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) Review Comment: No problem, thanks for reviewing the pr ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
soarez commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567497368 ## 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) Review Comment: I see. Thanks for clarifying -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567464576 ## 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) Review Comment: Thanks for your comment @soarez :smiley: ! 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567464576 ## 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) Review Comment: Thanks for your comment @soarez :smiley: ! 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 for tests to speed up for tests like e2e/integration tests which can't use MockTime as above. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567447761 ## core/src/test/scala/unit/kafka/log/LogManagerTest.scala: ## @@ -413,7 +413,7 @@ class LogManagerTest { assertEquals(numMessages * setSize / segmentBytes, log.numberOfSegments, "Check we have the expected number of segments.") // this cleanup shouldn't find any expired segments but should delete some to reduce size -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) Review Comment: > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
soarez commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567422789 ## 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) Review Comment: If the goal is to speed up tests, shouldn't we use a lower value here? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
showuon commented on code in PR #15719: URL: https://github.com/apache/kafka/pull/15719#discussion_r1567254740 ## 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) Review Comment: Also, we can set `atLeast(0)` in the defineInternal method. So that we don't need additional validator below. ## 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) Review Comment: 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. ## 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); Review Comment: 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`. ## core/src/test/scala/unit/kafka/log/LogManagerTest.scala: ## @@ -413,7 +413,7 @@ class LogManagerTest { assertEquals(numMessages * setSize / segmentBytes, log.numberOfSegments, "Check we have the expected number of segments.") // this cleanup shouldn't find any expired segments but should delete some to reduce size -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) Review Comment: 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-16552: Create an internal config to control InitialTaskDelayMs in LogManager to speed up tests [kafka]
brandboat opened a new pull request, #15719: URL: https://github.com/apache/kafka/pull/15719 related to KAFKA-16552, 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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org