You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/17 10:09:25 UTC
[GitHub] [kafka] kowshik opened a new pull request #9764: MINOR: Allow extra params in KafkaScheduler#scheduleOnce and use it in ZookeeperClient
kowshik opened a new pull request #9764:
URL: https://github.com/apache/kafka/pull/9764
In this PR, I've modified `KafkaScheduler#scheduleOnce` API to allow for couple extra params. This enables the API to be used in `KafkaScheduler#scheduleOnce`. This gets rid of the need to explicitly pass `period=-1L`, thus improving code readability a bit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik closed pull request #9764: MINOR: Eliminate KafkaScheduler#scheduleOnce in favor of KafkaScheduler#schedule
Posted by GitBox <gi...@apache.org>.
kowshik closed pull request #9764:
URL: https://github.com/apache/kafka/pull/9764
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a change in pull request #9764: MINOR: Allow extra params in KafkaScheduler#scheduleOnce and use it in ZooKeeperClient
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#discussion_r545288984
##########
File path: core/src/main/scala/kafka/utils/KafkaScheduler.scala
##########
@@ -99,8 +99,8 @@ class KafkaScheduler(val threads: Int,
}
}
- def scheduleOnce(name: String, fun: () => Unit): Unit = {
- schedule(name, fun, delay = 0L, period = -1L, unit = TimeUnit.MILLISECONDS)
+ def scheduleOnce(name: String, fun: () => Unit, delay: Long = 0L, unit: TimeUnit = TimeUnit.MILLISECONDS): Unit = {
Review comment:
Good point. Done.
##########
File path: core/src/main/scala/kafka/zookeeper/ZooKeeperClient.scala
##########
@@ -430,10 +430,10 @@ class ZooKeeperClient(connectString: String,
// Visibility for testing
private[zookeeper] def scheduleReinitialize(name: String, message: String, delayMs: Long): Unit = {
- reinitializeScheduler.schedule(name, () => {
+ reinitializeScheduler.scheduleOnce(name, () => {
Review comment:
Good point. Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #9764: MINOR: Eliminate KafkaScheduler#scheduleOnce in favor of KafkaScheduler#schedule
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#issuecomment-747741761
@chia7712: Thanks for the review! I've addressed your comments in b21762a.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9764: MINOR: Eliminate KafkaScheduler#scheduleOnce in favor of KafkaScheduler#schedule
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#issuecomment-747843386
just another thought. the ```period``` can change the behavior of ```schedule```. It seems to me we should NOT give the default value to it as it is error-prone. How about moving the ```scheduleOnce``` to parent and follow your first commit to add ```delay``` and ```unit``` to ```scheduleOnce```? WDYT
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #9764: MINOR: Allow extra params in KafkaScheduler#scheduleOnce and use it in ZookeeperClient
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#issuecomment-747344578
cc @junrao @chia7712 @hachikuji for review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #9764: MINOR: Eliminate KafkaScheduler#scheduleOnce in favor of KafkaScheduler#schedule
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#issuecomment-747849399
@chia7712: In general, either approach seems better than what we have in the code right now. In the original approach I had in the PR, we made the existing code better and improved readability. Then, in the approach you proposed we eliminated an API. So actually I don't have a strong preference for either of these, but if I were to pick one I'd go for my original approach that improved `scheduleOnce` with additional args since it helps readability.
Let me know.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9764: MINOR: Allow extra params in KafkaScheduler#scheduleOnce and use it in ZooKeeperClient
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#discussion_r545034745
##########
File path: core/src/main/scala/kafka/utils/KafkaScheduler.scala
##########
@@ -99,8 +99,8 @@ class KafkaScheduler(val threads: Int,
}
}
- def scheduleOnce(name: String, fun: () => Unit): Unit = {
- schedule(name, fun, delay = 0L, period = -1L, unit = TimeUnit.MILLISECONDS)
+ def scheduleOnce(name: String, fun: () => Unit, delay: Long = 0L, unit: TimeUnit = TimeUnit.MILLISECONDS): Unit = {
Review comment:
It seems to me we should remove ```scheduleOnce``` as it is equal to parent ```schedule`` (with default value)
see https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/utils/KafkaScheduler.scala#L56
##########
File path: core/src/main/scala/kafka/zookeeper/ZooKeeperClient.scala
##########
@@ -430,10 +430,10 @@ class ZooKeeperClient(connectString: String,
// Visibility for testing
private[zookeeper] def scheduleReinitialize(name: String, message: String, delayMs: Long): Unit = {
- reinitializeScheduler.schedule(name, () => {
+ reinitializeScheduler.scheduleOnce(name, () => {
Review comment:
How about calling ```schedule```? ```period``` and ```unit``` are equal to default value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9764: MINOR: Eliminate KafkaScheduler#scheduleOnce in favor of KafkaScheduler#schedule
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9764:
URL: https://github.com/apache/kafka/pull/9764#issuecomment-747851910
@kowshik Thanks for your response. You are right and the ```scheduleOnce``` is more readable. Also, I'd like to have following improvement.
1. remove default value of ```period``` (The callers should call ```scheduleOnce``` instead of setting -1 to period)
2. move scheduleOnce from ```KafkaScheduler``` to ```Scheduler``` ( it can have default implementation by calling ```schedule```)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org