Add schedule task enable/disable/trigger functionalities #2438
Conversation
Add schedule task enble/disable/trigger This is to fix wso2/product-micro-integrator#3567
WalkthroughAdds synchronized control APIs (activate, deactivate, trigger) to StartUpController that manage startup task state and persist it to the registry; relocates DynamicControlOperationResult from org.apache.synapse.inbound to org.apache.synapse.util and updates imports accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as StartUpController
participant Scheduler as StartupTask/Scheduler
participant Registry
Client->>Controller: activate()
alt controller initial/inactive
Controller->>Scheduler: start/activate task
Scheduler-->>Controller: started / throws error
Controller->>Registry: set state = ACTIVE
Registry-->>Controller: ack
Controller-->>Client: DynamicControlOperationResult(success=true)
else already active or error
Controller-->>Client: DynamicControlOperationResult(success=false, errMsg)
end
Client->>Controller: deactivate()
alt controller active
Controller->>Scheduler: stop/deactivate task
Scheduler-->>Controller: stopped / throws error
Controller->>Registry: set state = INACTIVE
Registry-->>Controller: ack
Controller-->>Client: DynamicControlOperationResult(success=true)
else already inactive or error
Controller-->>Client: DynamicControlOperationResult(success=false, errMsg)
end
Client->>Controller: trigger()
alt controller active
Controller->>Scheduler: trigger task execution
Scheduler-->>Controller: triggered / throws error
Controller-->>Client: DynamicControlOperationResult(success=true)
else inactive or error
Controller-->>Client: DynamicControlOperationResult(success=false, errMsg)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/core/src/main/java/org/apache/synapse/inbound/InboundEndpoint.java(1 hunks)modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java(6 hunks)modules/core/src/main/java/org/apache/synapse/util/DynamicControlOperationResult.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/core/src/main/java/org/apache/synapse/inbound/InboundEndpoint.java (1)
modules/core/src/main/java/org/apache/synapse/util/DynamicControlOperationResult.java (1)
DynamicControlOperationResult(21-37)
🔇 Additional comments (6)
modules/core/src/main/java/org/apache/synapse/inbound/InboundEndpoint.java (1)
34-34: LGTM! Import updated correctly.The import statement has been updated to reflect the new package location of
DynamicControlOperationResult.modules/core/src/main/java/org/apache/synapse/util/DynamicControlOperationResult.java (1)
19-37: LGTM! Package relocation is appropriate.Moving
DynamicControlOperationResultto theutilpackage is a good refactoring decision, as this class is now shared across multiple components (InboundEndpoint and StartUpController).modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java (4)
68-76: LGTM! Registry integration infrastructure is well-structured.The registry field, state enum, and constants are appropriately defined and follow the same pattern as
InboundEndpoint.
193-215: LGTM! Deactivation logic is correct.The
deactivate()method properly synchronizes access, delegates todeactivateTask(), and persists the state change to the registry on success.
258-265: LGTM! Registry state persistence is correctly implemented.The helper method properly handles null registry and persists the task state using appropriate registry APIs.
287-330: Verify if state restoration on initialization is needed.The registry is initialized at line 289, but unlike
InboundEndpoint(which restores state from registry instartInPausedMode()method at lines 529-535),StartUpControllerdoesn't appear to restore the persisted task state (ACTIVE/INACTIVE) when the controller is initialized after a server restart.This means:
- If a task is deactivated and the server restarts, the task may start in its default state rather than remaining deactivated
- The persisted state in the registry may not be honored on initialization
Please confirm whether state restoration is intentionally omitted or if it should be added to maintain task state across restarts.
If state restoration is needed, consider adding a method similar to
InboundEndpoint.startInPausedMode()that reads the state from the registry and conditionally callsdeactivateTask()during initialization.
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java
Show resolved
Hide resolved
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java (2)
264-271: Consider adding error handling for registry operations.The method lacks exception handling for
registry.newNonEmptyResource(). If the registry operation fails (e.g., due to connectivity issues, permissions, or resource conflicts), the exception would propagate to the caller, potentially causing the activate/deactivate operations to fail unexpectedly even though the task state change itself succeeded.Apply this diff to add defensive error handling:
private void setStartupControllerStateInRegistry(StartUpController.StartUpControllerState state) { if (Objects.isNull(registry)) { logger.warn("Registry not available! The state of the Task will not be saved."); return; } - registry.newNonEmptyResource(REG_STARTUP_CONTROLLER_BASE_PATH + getName(), false, "text/plain", - state.toString(), STARTUP_CONTROLLER_STATE); + try { + registry.newNonEmptyResource(REG_STARTUP_CONTROLLER_BASE_PATH + getName(), false, "text/plain", + state.toString(), STARTUP_CONTROLLER_STATE); + } catch (Exception e) { + logger.warn("Failed to persist task state to registry for Task: " + getName(), e); + } }
109-111: Consider thread safety for destroy() method.While the new
activate(),deactivate(), andtrigger()methods are properly synchronized, thedestroy()method is not. Ifdestroy()is called concurrently with any of the synchronized state-change methods, it could lead to race conditions where:
- A task is being activated/deactivated while being destroyed
- The task state becomes inconsistent
If concurrent destroy operations are possible in your deployment scenario (e.g., concurrent undeployment and state changes via API), consider adding synchronization:
- public void destroy() { + public synchronized void destroy() { destroy(true); }Additionally, consider making
destroy(boolean removeTask)synchronized:- public void destroy(boolean removeTask) { + public synchronized void destroy(boolean removeTask) { if (!destroyTask()) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java (1)
modules/core/src/main/java/org/apache/synapse/util/DynamicControlOperationResult.java (1)
DynamicControlOperationResult(21-37)
🔇 Additional comments (5)
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java (5)
34-35: LGTM!The imports are correct. The DynamicControlOperationResult package relocation from
org.apache.synapse.inboundtoorg.apache.synapse.utilaligns with the broader refactoring in this PR.
160-182: LGTM!The
activate()method is well-implemented with proper synchronization, error handling, and state persistence. The previous review feedback on variable naming consistency has been addressed.
193-215: LGTM!The
deactivate()method follows the same robust pattern asactivate()with proper synchronization, error handling, and state persistence.
224-252: LGTM!The
trigger()method correctly implements manual task execution with proper type safety checks (addressing previous review feedback). The decision not to persist state for a one-time trigger operation is appropriate.
293-336: Verify if task state should be restored from registry on initialization.The code persists task state (ACTIVE/INACTIVE) to the registry in
activate()anddeactivate()methods, butinit()doesn't read this state back to restore the previous state. This means:
- When the server restarts, tasks will revert to their default scheduled state (as defined in configuration)
- The state persisted in the registry becomes stale and unused
If the intent is to preserve enable/disable state across server restarts, you should add state restoration logic in
init(). If tasks should start fresh on each restart, this behavior is correct but should be documented.Based on the PR objectives to "enable/disable scheduled tasks," please clarify:
- Should task state persist across server restarts?
- Is the registry state intended for cluster coordination or just runtime tracking?
If state restoration is needed, consider adding this logic after line 295:
private void restoreTaskStateFromRegistry() { if (Objects.isNull(registry)) { return; } try { String statePath = REG_STARTUP_CONTROLLER_BASE_PATH + getName(); if (registry.resourceExists(statePath)) { String savedState = (String) registry.getResourceProperty(statePath, STARTUP_CONTROLLER_STATE); if (StartUpControllerState.INACTIVE.toString().equals(savedState)) { deactivateTask(); logger.info("Restored task [" + getName() + "] to INACTIVE state from registry."); } } } catch (Exception e) { logger.warn("Failed to restore task state from registry for Task: " + getName(), e); } }Then call it in
init()after task initialization (after line 311).
| private enum StartUpControllerState { | ||
| INITIAL, ACTIVE, INACTIVE | ||
| } | ||
|
|
||
| private static final String REG_STARTUP_CONTROLLER_BASE_PATH = "/repository/components/org.apache.synapse.startup/"; | ||
| private static final String STARTUP_CONTROLLER_STATE = "TASK_STATE"; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd modules/core/src/main/java/org/apache/synapse/startup/quartz && wc -l StartUpController.javaRepository: wso2/wso2-synapse
Length of output: 87
🏁 Script executed:
# Search for INITIAL state usage in StartUpController.java
rg -n "INITIAL" modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.javaRepository: wso2/wso2-synapse
Length of output: 97
🏁 Script executed:
# Find setStartupControllerStateInRegistry method
rg -n -A 10 "setStartupControllerStateInRegistry" modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.javaRepository: wso2/wso2-synapse
Length of output: 1873
🏁 Script executed:
# Search for task name assignments and validation
rg -n -C 3 "getName\(\)|setName|TaskDescription" modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.javaRepository: wso2/wso2-synapse
Length of output: 10296
🏁 Script executed:
# Check TaskDescription class for name validation
fd TaskDescription.java | head -5 | xargs -I {} wc -l {}Repository: wso2/wso2-synapse
Length of output: 137
🏁 Script executed:
# Search for where task names are used in registry paths
rg -n "REG_STARTUP_CONTROLLER_BASE_PATH" modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.javaRepository: wso2/wso2-synapse
Length of output: 292
🏁 Script executed:
# Examine TaskDescription class for name validation
fd TaskDescription.java -type f | xargs cat | head -100Repository: wso2/wso2-synapse
Length of output: 231
🏁 Script executed:
# Check if there's any name validation in TaskDescription setName method
rg -n -A 5 "setName\s*\(" modules/tasks/src/main/java/org/apache/synapse/task/TaskDescription.javaRepository: wso2/wso2-synapse
Length of output: 257
🏁 Script executed:
# Check the entire setName method and getName method
rg -n -B 2 -A 8 "public.*setName|public.*getName" modules/tasks/src/main/java/org/apache/synapse/task/TaskDescription.javaRepository: wso2/wso2-synapse
Length of output: 461
🏁 Script executed:
# Search for any task name validation or constraints in the codebase
rg -n "TaskDescription|task.*name.*valid|task.*name.*constraint" modules/tasks/src/main/java/org/apache/synapse/task/ --type java | grep -i "valid\|constraint\|sanitize\|name" | head -20Repository: wso2/wso2-synapse
Length of output: 3102
🏁 Script executed:
# Check the validateTaskDescription method
rg -n -A 15 "validateTaskDescription" modules/tasks/src/main/java/org/apache/synapse/task/TaskDescriptionRepository.javaRepository: wso2/wso2-synapse
Length of output: 761
🏁 Script executed:
# Also check if task names come from configuration files or user input
rg -n "TaskDescription\(\)|setName|taskName.*=" modules/core/src/main/java/org/apache/synapse/startup/ --type-list | head -30Repository: wso2/wso2-synapse
Length of output: 450
🏁 Script executed:
# Search for TaskDescription instantiation to see where names come from
rg -n -B 2 -A 2 "new TaskDescription" modules/ --type-list | head -50Repository: wso2/wso2-synapse
Length of output: 450
🏁 Script executed:
# Check validateName and validateUniqueness methods
rg -n -A 10 "private void validateName|private void validateUniqueness" modules/tasks/src/main/java/org/apache/synapse/task/TaskDescriptionRepository.javaRepository: wso2/wso2-synapse
Length of output: 756
🏁 Script executed:
# Search for where TaskDescription names are assigned in configuration parsing
rg -n "setName\|getName" modules/core/src/main/java/org/apache/synapse/config/xml/ | grep -i task | head -20Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Check if task names are parsed from XML configuration
fd -e xml modules/core/src/main/resources | head -5 | xargs grep -l "task\|startup" 2>/dev/null | head -3Repository: wso2/wso2-synapse
Length of output: 490
🏁 Script executed:
# Check for task configuration parsing to see if names come from user input
rg -n "task.*name\|Task.*name" modules/core/src/main/java/org/apache/synapse/config/xml/StartupFactoryDeserializer.javaRepository: wso2/wso2-synapse
Length of output: 189
🏁 Script executed:
# Look at how task startup elements are configured
fd StartupFactoryDeserializer.java | xargs cat | head -150Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Check Registry interface for constraints on resource paths
fd Registry.java | xargs cat | grep -A 5 "newNonEmptyResource\|interface\|class"Repository: wso2/wso2-synapse
Length of output: 3178
🏁 Script executed:
# Check what characters are allowed/constrained in registry paths
rg -n "path.*valid\|path.*sanitize\|special.*char" modules/ -i | head -20Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Look at the actual registry implementation details for path handling
fd -e java | xargs rg -l "newNonEmptyResource" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 15 "newNonEmptyResource" {}'Repository: wso2/wso2-synapse
Length of output: 3989
🏁 Script executed:
# Check if task names are used directly from user configuration (XML/YAML)
rg -n "taskName\|task.*name" modules/core/src/main/java/org/apache/synapse/config/ | grep -i "xml\|parse\|config" | head -15Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Check if there are any test cases or examples showing task names with special characters
rg -n "setName\|getName" modules/core/src/test modules/tasks/src/test -i | grep -i "task.*name\|special\|char\|path" | head -20Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Look for task configuration examples in test resources
fd -e xml modules/*/src/test/resources | xargs grep -l "task\|startup" | head -3 | xargs cat | grep -A 2 -B 2 "name" | head -40Repository: wso2/wso2-synapse
Length of output: 502
🏁 Script executed:
# Search for any error handling or issues with registry paths containing special characters
rg -n "resource.*path\|registry.*path" modules/core/src/main/java/org/apache/synapse/registry/ -A 3 | head -40Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Check if there are any path sanitization utilities in the codebase
rg -n "sanitize\|escape.*path\|encode.*path" modules/ -i | head -15Repository: wso2/wso2-synapse
Length of output: 43
🏁 Script executed:
# Look at where task names are parsed from configuration - likely XML parsers
fd StaxBuilder.java | xargs cat 2>/dev/null | grep -A 10 "task\|startup" | head -50Repository: wso2/wso2-synapse
Length of output: 43
Remove the unused INITIAL state or document its purpose.
The INITIAL state in the enum is never used in the code. Only ACTIVE and INACTIVE states are persisted to the registry. Either remove INITIAL if it's not needed, or document its purpose if it represents a default/uninitialized state.
Regarding registry path construction at line 269 (REG_STARTUP_CONTROLLER_BASE_PATH + getName()): while task name validation only enforces non-null and non-empty constraints without character restrictions, the current approach mirrors patterns used elsewhere in the codebase (e.g., ScheduledMessageProcessor, InboundEndpoint). If task names are controlled through configuration schemas, this may be acceptable, but consider documenting this assumption.
🤖 Prompt for AI Agents
In
modules/core/src/main/java/org/apache/synapse/startup/quartz/StartUpController.java
around lines 70-76, the enum StartUpControllerState declares INITIAL which is
never referenced and should be removed to avoid dead code, or if intended as a
default/uninitialized marker add a short Javadoc on the enum explaining its
purpose and where it would be used; additionally, add a brief comment near the
REG_STARTUP_CONTROLLER_BASE_PATH + getName() usage (around line ~269)
documenting the assumption that task names are validated via configuration
schemas and may contain characters used directly in registry paths (or introduce
explicit sanitization if that assumption is false).
1 participant
Add schedule task enble/disable/trigger
This is to fix wso2/product-micro-integrator#3567
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.