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.

Sign up for GitHub

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

Jump to bottom

Support cron timer to arrange the period heartbeat executor invoke time #16900

Merged
merged 10 commits into from Apr 14, 2023
Merged

Support cron timer to arrange the period heartbeat executor invoke time #16900

merged 10 commits into from Apr 14, 2023

Conversation

maobaolong
Copy link
Contributor

@maobaolong maobaolong commented Feb 17, 2023 •
edited

Support cron timer to arrange the period heartbeat executor invoke time.

Example

cron timer config: * 0-10,20-30,40-56 12-13 * * ? *

The heartbeat executor invoke at every minute from 0 through 10, from 20 through 30, from 40 through 56, at past every hour from 12 through 13.

@maobaolong maobaolong changed the title Support cron timer to arrange the period heartbeat executor invoke time. Add cron timer to arrange the period heartbeat executor invoke time. Feb 17, 2023
@maobaolong maobaolong changed the title Add cron timer to arrange the period heartbeat executor invoke time. Support cron timer to arrange the period heartbeat executor invoke time Feb 17, 2023
@Alluxio Alluxio deleted a comment from alluxio-bot Feb 17, 2023
@Alluxio Alluxio deleted a comment from alluxio-bot Feb 17, 2023
@Alluxio Alluxio deleted a comment from alluxio-bot Feb 17, 2023
@JiamingMai
Copy link
Contributor

@maobaolong checkstyle failed. Maybe you need to format the code style.

@JiamingMai JiamingMai added the type-feature This issue is a feature request label Feb 20, 2023
@jiacheliu3 jiacheliu3 added the area-master Alluxio master label Mar 3, 2023
@maobaolong
Copy link
Contributor Author

@JiamingMai The style issue has been resolved, PTAL.

dbw9580
dbw9580 requested changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@dbw9580 dbw9580 left a comment

Choose a reason for hiding this comment

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

Thanks for this interesting feature! Left some comments to improve the design.

core/common/src/main/java/alluxio/heartbeat/SleepingTimer.java Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/SleepingTimer.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/ScheduledTimer.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatTimer.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatThread.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatExecutor.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatExecutor.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatExecutor.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatExecutor.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/SleepingTimer.java Outdated Show resolved Hide resolved
@maobaolong
Copy link
Contributor Author

@dbw9580 Thanks advance for your helpful suggestion and kindly helping, I've refactor the code, PTAL.

dbw9580
dbw9580 requested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@dbw9580 dbw9580 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, only a few minor issues

core/common/src/main/java/alluxio/heartbeat/CronExpressionIntervalSupplier.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/FixedIntervalSupplier.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/HeartbeatTimer.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/heartbeat/ScheduledTimer.java Outdated Show resolved Hide resolved
core/common/src/test/java/alluxio/heartbeat/CronTimerTest.java Outdated Show resolved Hide resolved
@maobaolong maobaolong force-pushed the cronTimerHeartbeat branch 2 times, most recently from f53a617 to b902913 Compare April 2, 2023 15:47
@maobaolong
Copy link
Contributor Author

@dbw9580 The failed test has been fixed, PTAL.

@maobaolong maobaolong requested a review from dbw9580 April 7, 2023 04:59
dbw9580
dbw9580 reviewed Apr 7, 2023
View reviewed changes
...mmon/src/test/java/alluxio/heartbeat/SleepingTimerForCronExpressionIntervalSupplierTest.java
() -> {
try {
return new CronExpressionIntervalSupplier(
new CronExpression("* 30-59 0-1,4-9,13-23 * * ? *"), INTERVAL_MS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is not the standard crontab expression. When we add the docs, a link to the cron syntax specification should be included. https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/apache/logging/log4j/core/util/CronExpression.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we use log4j's implementation of cron? Is this cron syntax commonly used by log4j or other utilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbw9580 Log4j cron expression is used for config RollingFileAppender and TimeBasedRollingPolicy, it can control the rolling of log file, something like

appender.rolling.type = RollingFile
appender.rolling.name = RollingFile
appender.rolling.fileName = logs/app.log
appender.rolling.filePattern = logs/app-%d{yyyy-MM-dd}.log
appender.rolling.layout.type = PatternLayout
appender.rolling.layout.pattern = %d{yyyy-MM-dd HH:mm:ss} [%t] %-5p %c{1}:%L - %m%n
appender.rolling.policies.type = Policies
appender.rolling.policies.time.type = TimeBasedTriggeringPolicy
appender.rolling.policies.time.interval = 1
appender.rolling.policies.time.modulate = true
appender.rolling.policies.time.cron = 0 0 1 * * ?

dbw9580
dbw9580 approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@dbw9580 dbw9580 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiacheliu3
Copy link
Contributor

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit fd19fb6 into Alluxio:master Apr 14, 2023
19 checks passed
jiacheliu3
jiacheliu3 reviewed Apr 21, 2023
View reviewed changes
core/common/src/main/java/alluxio/heartbeat/HeartbeatThread.java
mStatus = Status.RUNNING;
mExecutor.heartbeat();
LOG.debug("{} #{} will run limited in {}s", mThreadName, counter++, limitTime / 1000);
mExecutor.heartbeat(limitTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Who stops this heartbeat if the limitTime has been exceeded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stopped by Heartbeat implementation

jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 19, 2023
@maobaolong @jiacheliu3
cherry pick and resolve conflicts: Support cron timer to arrange the …
2d59e6b 
…period heartbeat executor invoke time

Support cron timer to arrange the period heartbeat executor invoke time.

Example

```
cron timer config: * 0-10,20-30,40-56 12-13 * * ? *

The heartbeat executor invoke at every minute from 0 through 10, from 20 through 30, from 40 through 56, at past every hour from 12 through 13.
```

			pr-link: Alluxio#16900
			change-id: cid-9277f30e2159e64863067d14cbcbee526707c5b6
zhezhidashi pushed a commit to zhezhidashi/alluxio-zhaozihao that referenced this pull request May 23, 2023
@maobaolong @zhezhidashi
cherry pick and resolve conflicts: Support cron timer to arrange the …
53e8209 
…period heartbeat executor invoke time

Support cron timer to arrange the period heartbeat executor invoke time.

Example

```
cron timer config: * 0-10,20-30,40-56 12-13 * * ? *

The heartbeat executor invoke at every minute from 0 through 10, from 20 through 30, from 40 through 56, at past every hour from 12 through 13.
```

			pr-link: Alluxio#16900
			change-id: cid-9277f30e2159e64863067d14cbcbee526707c5b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiacheliu3 jiacheliu3 jiacheliu3 left review comments

@dbw9580 dbw9580 dbw9580 approved these changes

Assignees

@tcrain tcrain

Labels
area-master Alluxio master type-feature This issue is a feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@maobaolong @JiamingMai @jiacheliu3 @dbw9580 @tcrain @alluxio-bot