feat: make executor timeout configurable via server config#6285
Open
NETIZEN-11 wants to merge 2 commits intomindersec:mainfrom
Open
feat: make executor timeout configurable via server config#6285NETIZEN-11 wants to merge 2 commits intomindersec:mainfrom
NETIZEN-11 wants to merge 2 commits intomindersec:mainfrom
Conversation
- Add ExecutionTimeout field to EventConfig with 5m default - Update ExecutorEventHandler to accept and use configurable timeout - Implement fallback to DefaultExecutionTimeout for invalid values - Add logging for effective timeout during initialization - Update service layer to pass config timeout to handler - Add comprehensive tests for timeout configuration and fallback - Update example config with execution_timeout documentation Fixes mindersec#6282 Signed-off-by: NETIZEN-11 <kumarnitesh121411@gmail.com>
|
|
Member
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
Thanks for this. Could you coordinate with #6278 (and the comments there), since this seems to be a duplicate of that PR?
In particular, see the comment on #6278 (comment) about testing the timeout / cancellation behavior with a small requested timeout of e.g. 1-5 seconds, and a task which would try to execute for longer?
Member
|
Hello @NETIZEN-11 -- are you likely to return to this PR? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #6282
This PR makes the executor timeout configurable via server configuration instead of being hardcoded. Previously, the executor relied on a fixed
DefaultExecutionTimeout, which limited flexibility across different environments (e.g., development, CI, production).With this change, users can now control execution timeout behavior through configuration without modifying code.
Changes
ExecutionTimeoutfield toEventConfig(default: 5m)ExecutorEventHandlerto use configurable timeoutDefaultExecutionTimeoutfor invalid values (≤ 0)Behavior
execution_timeout> 0 → use configured valueexecution_timeout≤ 0 or missing → fallback toDefaultExecutionTimeoutConfiguration Example
Testing
All existing tests pass
Added
TestNewExecutorEventHandler_TimeoutConfigurationCovered:
Custom timeout usage
Zero value fallback
Negative value fallback
No regressions observed
Build and lint checks pass
Notes
Checklist