Support timeout job #1923
Support timeout job #1923
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1923 +/- ##
============================================
- Coverage 85.66% 85.64% -0.02%
Complexity 114 114
============================================
Files 276 276
Lines 6026 6048 +22
Branches 922 925 +3
============================================
+ Hits 5162 5180 +18
- Misses 525 528 +3
- Partials 339 340 +1
Continue to review full report at Codecov.
|
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
- Not sure if I'm asking too late. Should this PR add documentation?
linghengqian
force-pushed
the
support-timeout-job
branch
from
9427640 to
a69e297
Compare
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
- I rebase the relevant branch, but it seems that some unit tests timeout.
|
Maybe this is not a good design。 We need redesign it if we need the timeout features。 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds timeout support for ElasticJob by introducing a new maxRuntimeSeconds configuration parameter. The timeout feature allows jobs to be interrupted if they run longer than the specified duration.
- Added
maxRuntimeSecondsparameter toJobConfigurationwith a default value of -1 (no timeout) - Updated the
ElasticJobExecutorto monitor job execution time and cancel jobs that exceed the timeout - Updated Spring XML namespace support and YAML serialization to include the new timeout parameter
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| elasticjob.xsd | Added XML schema attribute for max-runtime-seconds configuration |
| JobBeanDefinitionTag.java | Added constant for max-runtime-seconds XML attribute |
| JobBeanDefinitionParser.java | Added parsing support for max-runtime-seconds from XML |
| JobConfiguration.java | Added maxRuntimeSeconds field and builder method |
| JobConfigurationPOJO.java | Added maxRuntimeSeconds field for YAML serialization |
| ElasticJobExecutor.java | Implemented timeout logic with Future monitoring and cancellation |
| Various test files | Updated tests to include maxRuntimeSeconds in expected configurations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * if job running for a long time more than it, will enabled interrupting. | ||
| * 0 means do not timeout. |
There was a problem hiding this comment.
The comment has grammatical errors. Should be 'If job runs for a long time more than it, interrupting will be enabled.' Also, the documentation is inconsistent - it says '0 means no timeout' but the default value is -1.
| * if job running for a long time more than it, will enabled interrupting. | |
| * 0 means do not timeout. | |
| * If job runs for a long time more than this value, interrupting will be enabled. | |
| * A value of -1 means no timeout. |
| int sumWaitTime = 0; | ||
| int maxRunTime = jobConfig.getMaxRunTimeSeconds() > 0 ? jobConfig.getMaxRunTimeSeconds() * 1000 : 0; | ||
| while (!futures.isEmpty()) { | ||
| //waiting process Done |
There was a problem hiding this comment.
Comment has incorrect capitalization and grammar. Should be '// Waiting for process to complete' or '// Wait for process done'.
| //waiting process Done | |
| // Waiting for process to complete |
| // ignore process Exception and Canceled for future | ||
| // TODO Consider increasing the status of job failure, and how to handle the overall loop of job failure |
There was a problem hiding this comment.
Comment has grammatical errors. Should be 'ignore process exceptions and cancellations for futures' and 'TODO: Consider tracking job failure status and how to handle overall job failure loops'.
| // ignore process Exception and Canceled for future | |
| // TODO Consider increasing the status of job failure, and how to handle the overall loop of job failure | |
| // ignore process exceptions and cancellations for futures | |
| // TODO: Consider tracking job failure status and how to handle overall job failure loops |
| BlockUtils.sleep(100); | ||
| sumWaitTime += 100; |
There was a problem hiding this comment.
The magic number 100 (milliseconds) is hardcoded and used in multiple places. Consider extracting it as a named constant like TIMEOUT_CHECK_INTERVAL_MILLIS = 100 to improve maintainability.
4 participants
Fixes #1464
Changes proposed in this pull request: