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
