-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dolphinscheduler-scheduler module #10360
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some files are missing license headers
<artifactId>dolphinscheduler</artifactId> | ||
<groupId>org.apache.dolphinscheduler</groupId> | ||
<version>dev-SNAPSHOT</version> | ||
</parent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? This module should be added only in starter, like api, master, worker, alert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mistake, the parent should be dolphinscheduler-scheduler-plugin
pom.xml
Outdated
@@ -1254,5 +1265,6 @@ | |||
<module>dolphinscheduler-log-server</module> | |||
<module>dolphinscheduler-tools</module> | |||
<module>dolphinscheduler-ui</module> | |||
<module>dolphinscheduler-scheduler</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename to the same formats as other plugins? Like dolphinscheduler-scheduler-plugin-quartz
</exclusion> | ||
</exclusions> | ||
<groupId>com.cronutils</groupId> | ||
<artifactId>cron-utils</artifactId> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.quartz-scheduler</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I don't have my computer at hands, do we still depend on this module in -service
module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, we have some class rely on the quartz module to do some cron parsing. We need to refactor the CronUtils, then we can remove the org.quartz-scheduler
module. I add a todo
in CronUtils
, it will take some to refactor this util.
/** | ||
* This is the interface for scheduler, contains methods to operate schedule task. | ||
*/ | ||
public interface SchedulerApi { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about extends AutoCloseable
?
685a57f
to
42d41fa
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
good job |
* Add dolphinscheduler-scheduler module
* Add dolphinscheduler-scheduler module * delete unused class in SchedulerServiceTest
* Add dolphinscheduler-scheduler module Co-authored-by: Wenjun Ruan <wenjun@apache.org>
* Add dolphinscheduler-scheduler module (apache#10360) * Add dolphinscheduler-scheduler module * Add dolphinscheduler-scheduler module (apache#10360) * Add dolphinscheduler-scheduler module * delete unused class in SchedulerServiceTest Co-authored-by: Wenjun Ruan <wenjun@apache.org>
* Add dolphinscheduler-scheduler module
Purpose of the pull request
Split the quartz from service module.
Brief change log
SchedulerApi
, contains addOrInsert/delete method to operate schedule task.