[UNOMI-897] Groovy data corruption and performance fixes#720
Conversation
…nDispatcher - Fixed data corruption issue by removing shared GroovyShell instance with separate script instances for separate variables - Improved performance by moving to pre-compilation of scripts to avoid on-the-fly and previously systematic compilation of Groovy scripts - Add ScriptMetadata class for script management
…nDispatcher - Fixed data corruption issue by removing shared GroovyShell instance with separate script instances for separate variables - Improved performance by moving to pre-compilation of scripts to avoid on-the-fly and previously systematic compilation of Groovy scripts - Add ScriptMetadata class for script management
|
@jayblanc or @jsinovassin would you mind reviewing this PR ? |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses data corruption and performance issues in the Groovy action dispatching by removing the shared GroovyShell, introducing pre-compilation and caching of scripts, and updating the public API and tests accordingly.
- Removed shared
GroovyShellin favor of thread-safe, per-script compiled classes. - Added
ScriptMetadatafor hash-based change detection and caching. - Updated service interface and integration tests to use
getCompiledScriptand metadata APIs.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| itests/.../GroovyActionsServiceIT.java | Updated integration tests to call getCompiledScript instead of legacy APIs. |
| GroovyActionsServiceImpl.java | Refactored core service to precompile scripts, manage ScriptMetadata, and remove shared shell. |
| GroovyActionsService.java | Extended service interface with new methods (getOrCompileScript, getCompiledScript, getScriptMetadata). |
| ScriptMetadata.java | Added immutable class for storing compiled script metadata and change detection. |
| GroovyActionDispatcher.java | Modified dispatcher to instantiate and execute precompiled Script classes. |
Comments suppressed due to low confidence (3)
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java:92
- The return type is broader than other methods. Consider narrowing this to Class<? extends Script> for consistency with getCompiledScript.
Class<?> getOrCompileScript(String id);
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java:106
- [nitpick] The parameter name 'id' is inconsistent with other methods that use 'actionName'. Consider renaming it to 'actionName' for a consistent API.
Class<? extends Script> getCompiledScript(String id);
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java:180
- New pre-compilation logic introduced here should be covered by unit tests to verify correct caching behavior and error handling.
private void preloadAllScripts() {
- Standardized API parameter naming on actionName - Added logic to avoid logging compilation error on refreshes multiple times - Removed the unused getOrCompileScript method
UNOMI-897: Fix data corruption and performance issues in GroovyActionDispatcher