Skip to content

Remove ScheduleUtil，use the existing CronUtils#1904

Merged
lfyee merged 8 commits intoapache:devfrom
lfyee:dev
Feb 7, 2020
Merged

Remove ScheduleUtil，use the existing CronUtils#1904
lfyee merged 8 commits intoapache:devfrom
lfyee:dev

Conversation

@lfyee
Copy link
Copy Markdown
Contributor

@lfyee lfyee commented Feb 7, 2020

What is the purpose of the pull request

CronUtils.java is a public quartz tools, so delete the ScheduleUtil.java, then the project dolphinscheduler-api can remove dolphinscheduler-server

Verify this pull request

the command 'mvn clean package -Dmaven.test.skip=true' is success

  • *remove ScheduleUtilsTest
  • *remove ScheduleUtils
  • *edit ExecutorService for use CronUtils
  • *edit MasterExecThread for use CronUtils

Remove ScheduleUtil，use the existing CronUtils
662233e 
CronUtils is a public quartz tools, then the project dolphinscheduler-api can remove dolphinscheduler-server
@Jave-Chen
Copy link
Copy Markdown
Contributor

Jave-Chen commented Feb 7, 2020 

CronExpression cronExpression = null;
try {
    cronExpression = CronUtils.parse2CronExpression(item.getCrontab());
    List<Date> list = CronUtils.getSelfFireDateList(start, end, cronExpression);
    listDate.addAll(list);
} catch (ParseException e) {
    logger.error(e.getMessage(), e);
    continue;
}

These code are duplicated, is it possible to encapsulate then as a method?

Optimize duplicate code
9d944d3 
Use public methods to simplify duplicate code
@lfyee
Copy link
Copy Markdown
Contributor Author

lfyee commented Feb 7, 2020

@Jave-Chen ,use public methods to simplify duplicate code

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 7, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (dev@9d944d3). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev    #1904   +/-   ##
======================================
  Coverage       ?   24.27%           
======================================
  Files          ?      298           
  Lines          ?    14388           
  Branches       ?     2371           
======================================
  Hits           ?     3493           
  Misses         ?    10467           
  Partials       ?      428
Impacted Files Coverage Δ
...er/server/worker/task/AbstractCommandExecutor.java 40.5% <ø> (ø)
...che/dolphinscheduler/dao/utils/cron/CronUtils.java 90.47% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d944d3...0eec3b8. Read the comment docs.

@Jave-Chen
Copy link
Copy Markdown
Contributor

Jave-Chen commented Feb 7, 2020

Add include record at incubator-dolphinscheduler/pom.xml will get right UT coverage.

@Jave-Chen
Copy link
Copy Markdown
Contributor

Jave-Chen commented Feb 7, 2020

+1 good job

Jave-Chen
Jave-Chen reviewed Feb 7, 2020
View reviewed changes
dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/cron/CronUtilsTest.java
// strCrontab = "0/50 0/59 * * * ? *";
// strCrontab = "3/5 * 0/5 * * ? *";
// strCrontab = "1/5 3/5 1/5 3/30 * ? *";

Copy link
Copy Markdown
Contributor

@Jave-Chen Jave-Chen Feb 7, 2020

Choose a reason for hiding this comment

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

I think 73-75 line can be removed.

Copy link
Copy Markdown
Contributor Author

@lfyee lfyee Feb 7, 2020

Choose a reason for hiding this comment

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

ok, removed

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Feb 7, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

88.9% 88.9% Coverage
0.0% 0.0% Duplication

@lfyee lfyee requested a review from Technoboy- February 7, 2020 11:40
@lfyee lfyee merged commit 4a25127 into apache:dev Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jave-Chen Jave-Chen Jave-Chen approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lfyee @Jave-Chen @codecov-io @Technoboy-