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
