jsinovassin commented on code in PR #720:
URL: https://github.com/apache/unomi/pull/720#discussion_r2216021184
##########
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java:
##########
@@ -209,48 +328,182 @@ private void saveActionType(Action action) {
definitionsService.setActionType(actionType);
}
+ /**
+ * {@inheritDoc}
+ */
@Override
public void remove(String id) {
- if (groovyCodeSourceMap.containsKey(id)) {
- try {
- definitionsService.removeActionType(
-
groovyShell.parse(groovyCodeSourceMap.get(id)).getClass().getMethod("execute").getAnnotation(Action.class).id());
- } catch (NoSuchMethodException e) {
- LOGGER.error("Failed to delete the action type for the id {}",
id, e);
+ validateNotEmpty(id, "Script ID");
+
+ LOGGER.info("Removing script: {}", id);
+
+ ScriptMetadata removedMetadata = scriptMetadataCache.remove(id);
+ persistenceService.remove(id, GroovyAction.class);
+
+ if (removedMetadata != null) {
+ Action actionAnnotation =
getActionAnnotation(removedMetadata.getCompiledClass());
+ if (actionAnnotation != null) {
+ definitionsService.removeActionType(actionAnnotation.id());
}
- persistenceService.remove(id, GroovyAction.class);
}
+
+ LOGGER.info("Script {} removed successfully", id);
}
+
+ /**
+ * {@inheritDoc}
+ * Performance Warning: Compiles on-demand if not cached.
+ */
@Override
- public GroovyCodeSource getGroovyCodeSource(String id) {
- return groovyCodeSourceMap.get(id);
+ public Class<? extends Script> getOrCompileScript(String id) {
+ validateNotEmpty(id, "Script ID");
+
+ ScriptMetadata metadata = scriptMetadataCache.get(id);
+ if (metadata != null) {
+ return metadata.getCompiledClass();
+ }
+
+ long startTime = System.currentTimeMillis();
+ LOGGER.warn("Script {} not found in cache, compiling on-demand
(performance warning)", id);
+
+ GroovyAction groovyAction = persistenceService.load(id,
GroovyAction.class);
+ if (groovyAction == null) {
+ LOGGER.warn("Script {} not found in persistence, returning null
({}ms)", id,
+ System.currentTimeMillis() - startTime);
+ return null;
+ }
+
+ try {
+ long compilationStartTime = System.currentTimeMillis();
+ ScriptMetadata newMetadata = compileAndCreateMetadata(id,
groovyAction.getScript());
+ long compilationTime = System.currentTimeMillis() -
compilationStartTime;
+
+ scriptMetadataCache.put(id, newMetadata);
+
+ long totalTime = System.currentTimeMillis() - startTime;
+ LOGGER.warn("On-demand compilation completed for {} (total: {}ms,
compilation: {}ms)",
+ id, totalTime, compilationTime);
+ return newMetadata.getCompiledClass();
+ } catch (Exception e) {
+ long totalTime = System.currentTimeMillis() - startTime;
+ LOGGER.error("Failed to compile script {} on-demand ({}ms)", id,
totalTime, e);
+ return null;
+ }
}
/**
- * Build a GroovyCodeSource object and add it to the class loader of the
groovyScriptEngine
- *
- * @param groovyScript groovy script as a string
- * @param actionName Name of the action
- * @return Built GroovyCodeSource
+ * {@inheritDoc}
+ */
+ @Override
+ public Class<? extends Script> getCompiledScript(String id) {
+ validateNotEmpty(id, "Script ID");
+
+ ScriptMetadata metadata = scriptMetadataCache.get(id);
+ if (metadata == null) {
+ LOGGER.warn("Script {} not found in cache", id);
+ return null;
+ }
+ return metadata.getCompiledClass();
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public ScriptMetadata getScriptMetadata(String actionName) {
+ validateNotEmpty(actionName, "Action name");
+
+ return scriptMetadataCache.get(actionName);
+ }
+
+ /**
+ * Creates GroovyCodeSource for compilation.
*/
private GroovyCodeSource buildClassScript(String groovyScript, String
actionName) {
return new GroovyCodeSource(groovyScript, actionName,
"/groovy/script");
}
+ /**
+ * Persists script to storage.
+ */
private void saveScript(String actionName, String script) {
GroovyAction groovyScript = new GroovyAction(actionName, script);
persistenceService.save(groovyScript);
LOGGER.info("The script {} has been persisted.", actionName);
}
+ /**
+ * Refreshes scripts from persistence with selective recompilation.
+ * Uses hash-based change detection and atomic cache updates.
+ */
private void refreshGroovyActions() {
- Map<String, GroovyCodeSource> refreshedGroovyCodeSourceMap = new
HashMap<>();
-
persistenceService.getAllItems(GroovyAction.class).forEach(groovyAction ->
refreshedGroovyCodeSourceMap
- .put(groovyAction.getName(),
buildClassScript(groovyAction.getScript(), groovyAction.getName())));
- groovyCodeSourceMap = refreshedGroovyCodeSourceMap;
+ long startTime = System.currentTimeMillis();
+
+ Map<String, ScriptMetadata> newMetadataCache = new
ConcurrentHashMap<>();
+ int unchangedCount = 0;
+ int recompiledCount = 0;
+ int errorCount = 0;
+ long totalCompilationTime = 0;
+
+ for (GroovyAction groovyAction :
persistenceService.getAllItems(GroovyAction.class)) {
+ String actionName = groovyAction.getName();
+ String scriptContent = groovyAction.getScript();
+
+ try {
+ ScriptMetadata existingMetadata =
scriptMetadataCache.get(actionName);
+ if (existingMetadata != null &&
!existingMetadata.hasChanged(scriptContent)) {
+ newMetadataCache.put(actionName, existingMetadata);
+ unchangedCount++;
+ LOGGER.debug("Script {} unchanged during refresh, keeping
cached version", actionName);
+ } else {
+ if (recompiledCount == 0) {
+ LOGGER.info("Refreshing scripts from persistence
layer...");
+ }
+
+ long compilationStartTime = System.currentTimeMillis();
+ ScriptMetadata metadata =
compileAndCreateMetadata(actionName, scriptContent);
+ long compilationTime = System.currentTimeMillis() -
compilationStartTime;
+ totalCompilationTime += compilationTime;
+
+ newMetadataCache.put(actionName, metadata);
+ recompiledCount++;
+ LOGGER.info("Script {} recompiled during refresh ({}ms)",
actionName, compilationTime);
+ }
+
+ } catch (Exception e) {
+ if (errorCount == 0 && recompiledCount == 0) {
+ LOGGER.info("Refreshing scripts from persistence
layer...");
+ }
+
+ errorCount++;
+ LOGGER.error("Failed to refresh script: {}", actionName, e);
+
+ ScriptMetadata existingMetadata =
scriptMetadataCache.get(actionName);
+ if (existingMetadata != null) {
+ newMetadataCache.put(actionName, existingMetadata);
+ LOGGER.warn("Keeping existing version of script {} due to
compilation error", actionName);
+ }
+ }
+ }
+
+ this.scriptMetadataCache = newMetadataCache;
+
+ if (recompiledCount > 0 || errorCount > 0) {
+ long totalTime = System.currentTimeMillis() - startTime;
+ LOGGER.info("Script refresh completed: {} unchanged, {}
recompiled, {} errors. Total time: {}ms",
Review Comment:
By default the refresh is done each 1 seconde. If one of the script is in
error, the log will be displayed each seconds. It can lead to log pollution.
Maybe we can separate the case where recompiledCount > 0 and errorCount > 0
as an error will already be displayed in the catch above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@unomi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
