Skip to content
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

Directly use Quartz replase api to bootstrap a schedule #15781

Merged

Conversation

ruanwenjun
Copy link
Member

Purpose of the pull request

Optimize the scheduler code in quartz plugin, use quartz replace API to insert/update trigger and job.

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_directlyUseQuartzReplaceAPI branch 3 times, most recently from 1f6f6b1 to 681aec2 Compare March 29, 2024 11:48
@qingwli
Copy link
Member

qingwli commented Mar 30, 2024

It's ok to remove lock when build job details and schedule?

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_directlyUseQuartzReplaceAPI branch from 1437979 to e43c7dc Compare March 30, 2024 02:52
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_directlyUseQuartzReplaceAPI branch from e43c7dc to bcfd0da Compare March 30, 2024 03:21
@ruanwenjun
Copy link
Member Author

ruanwenjun commented Mar 30, 2024

It's ok to remove lock when build job details and schedule?

We don't need to use lock here, and quartz will use db lock when dealing with job/trigger.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_directlyUseQuartzReplaceAPI branch 5 times, most recently from e9e8d49 to 713add8 Compare March 30, 2024 07:12
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2024

Codecov Report

Attention: Patch coverage is 31.00000% with 69 lines in your changes are missing coverage. Please review.

Project coverage is 39.11%. Comparing base (920ac15) to head (e727d8a).

❗ Current head e727d8a differs from pull request most recent head edf967e. Consider uploading reports for the commit edf967e to get more accurate results

Files Patch % Lines
...ler/scheduler/quartz/QuartzCornTriggerBuilder.java 0.00% 26 Missing ⚠️
...hinscheduler/scheduler/quartz/QuartzScheduler.java 0.00% 19 Missing ⚠️
...duler/scheduler/quartz/QuartzJobDetailBuilder.java 0.00% 15 Missing ⚠️
...cheduler/scheduler/quartz/ProcessScheduleTask.java 0.00% 4 Missing ⚠️
...lphinscheduler/scheduler/quartz/QuartzJobData.java 75.00% 2 Missing and 2 partials ⚠️
...scheduler/quartz/QuartzSchedulerConfiguration.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15781      +/-   ##
============================================
- Coverage     39.19%   39.11%   -0.08%     
- Complexity     4876     4888      +12     
============================================
  Files          1317     1326       +9     
  Lines         45039    45206     +167     
  Branches       4808     4818      +10     
============================================
+ Hits          17652    17684      +32     
- Misses        25501    25635     +134     
- Partials       1886     1887       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SbloodyS SbloodyS added improvement make more easy to user or prompt friendly 3.3.0 labels Mar 31, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Mar 31, 2024
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_directlyUseQuartzReplaceAPI branch from 713add8 to 5c0a1a8 Compare March 31, 2024 14:56
Copy link
Member

@qingwli qingwli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

sonarcloud bot commented Apr 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
30.2% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@ruanwenjun ruanwenjun merged commit 0419543 into apache:dev Apr 2, 2024
59 of 60 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_directlyUseQuartzReplaceAPI branch April 2, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.3.0 backend improvement make more easy to user or prompt friendly ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants