[
https://issues.apache.org/jira/browse/KAFKA-597?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13526966#comment-13526966
]
Joel Koshy commented on KAFKA-597:
----------------------------------
I haven't fully reviewed, but a couple of initial comments:
- I think the javadoc on KafkaScheduler's daemon param is a bit misleading as
it currently suggests that daemon=true would prevent the VM from shutting down.
- The patch inverts the daemon flag on some of the existing usages of
KafkaScheduler - i.e., daemon now defaults to true and there are some places
where daemon was false. We would need to survey these usages and identify
whether it makes sense to keep them non-daemon or not.
- The other question is on shutdownNow: the previous scheduler allowed the
relaxed shutdown - i.e., don't interrupt threads that are currently executing.
This change forces all shutdowns to use shutdownNow. Question is whether there
are existing tasks that need to complete that would not tolerate an interrupt.
I'm not sure about that - we'll need to look at existing usages. E.g.,
KafkaServer's kafkaScheduler used the shutdown() method - now it's effectively
shutdownNow.
> Refactor KafkaScheduler
> -----------------------
>
> Key: KAFKA-597
> URL: https://issues.apache.org/jira/browse/KAFKA-597
> Project: Kafka
> Issue Type: Bug
> Affects Versions: 0.8.1
> Reporter: Jay Kreps
> Priority: Minor
> Attachments: KAFKA-597-v1.patch, KAFKA-597-v2.patch,
> KAFKA-597-v3.patch, KAFKA-597-v4.patch
>
>
> It would be nice to cleanup KafkaScheduler. Here is what I am thinking
> Extract the following interface:
> trait Scheduler {
> def startup()
> def schedule(fun: () => Unit, name: String, delayMs: Long = 0, periodMs:
> Long): Scheduled
> def shutdown(interrupt: Boolean = false)
> }
> class Scheduled {
> def lastExecution: Long
> def cancel()
> }
> We would have two implementations, KafkaScheduler and MockScheduler.
> KafkaScheduler would be a wrapper for ScheduledThreadPoolExecutor.
> MockScheduler would only allow manual time advancement rather than using the
> system clock, we would switch unit tests over to this.
> This change would be different from the existing scheduler in a the following
> ways:
> 1. Would not return a ScheduledFuture (since this is useless)
> 2. shutdown() would be a blocking call. The current shutdown calls, don't
> really do what people want.
> 3. We would remove the daemon thread flag, as I don't think it works.
> 4. It returns an object which let's you cancel the job or get the last
> execution time.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
