Alon Bar-Lev has posted comments on this change. Change subject: aaa: WIP - New Extensions Manager based on the extensions API ......................................................................
Patch Set 1: (13 comments) http://gerrit.ovirt.org/#/c/26427/1/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManagerNew.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManagerNew.java: Line 39: private boolean enabled; Line 40: private boolean activated; Line 41: private Extension extension; Line 42: private ExtMap context; Line 43: private Logger logger = log; decide logger or log ? oh... you get it from outside context? please avoid if possible. but... you do not need this... as you should hold the logger within the context of extension, so that others can use the invoke wrapper with a reference to extension instance only. Line 44: Line 45: public ExtensionEntry(Properties props, File file) { Line 46: this.file = file; Line 47: load(props); Line 88: } Line 89: Line 90: } Line 91: Line 92: private static final Logger log = LoggerFactory.getLogger(ExtensionsManager.class); add: private static final Logger dumpLogger = LoggerFactory.getLogger(log.getName() + ".dump"); Line 93: private static volatile ExtensionsManagerNew instance = null; Line 94: private Map<String, ExtensionEntry> loadedEntries = new HashMap<>(); Line 95: private Map<String, Module> loadedModules = new HashMap<>(); Line 96: Line 132: } Line 133: return result; Line 134: } Line 135: Line 136: private ExtensionsManagerNew() { it will really be better to send patches for ExtensionManager so we see changes... Line 137: for (File directory : EngineLocalConfig.getInstance().getExtensionsDirectories()) { Line 138: if (!directory.exists()) { Line 139: log.warn(String.format("The directory '%1$s' cotaning configuration files does not exist.", Line 140: directory.getAbsolutePath())); Line 189: //Activate the extension Line 190: if (entry.enabled && entry.extension == null) { Line 191: try { Line 192: entry.context = new ExtMap(); Line 193: entry.context.put(ContextKeys.CONFIGURATION, props); you can use: entry.context = new ExtMap().mput( ).mput( ).mput( )... Line 194: entry.context.put(ContextKeys.INSTANCE_NAME, props.getProperty(ConfigKeys.NAME) != null ? props.getProperty(ConfigKeys.NAME) : String.format("INSTANCE_NAME_%1$s", RandomUtils.nextInt())); Line 195: entry.context.put(ContextKeys.PROVIDES, props.getProperty(ConfigKeys.PROVIDES) != null ? props.getProperty(ConfigKeys.PROVIDES) : ""); Line 196: Line 197: entry.extension = (Extension) lookupService( Line 190: if (entry.enabled && entry.extension == null) { Line 191: try { Line 192: entry.context = new ExtMap(); Line 193: entry.context.put(ContextKeys.CONFIGURATION, props); Line 194: entry.context.put(ContextKeys.INSTANCE_NAME, props.getProperty(ConfigKeys.NAME) != null ? props.getProperty(ConfigKeys.NAME) : String.format("INSTANCE_NAME_%1$s", RandomUtils.nextInt())); you can just use sequence with prefix :)) Line 195: entry.context.put(ContextKeys.PROVIDES, props.getProperty(ConfigKeys.PROVIDES) != null ? props.getProperty(ConfigKeys.PROVIDES) : ""); Line 196: Line 197: entry.extension = (Extension) lookupService( Line 198: Extension.class, Line 191: try { Line 192: entry.context = new ExtMap(); Line 193: entry.context.put(ContextKeys.CONFIGURATION, props); Line 194: entry.context.put(ContextKeys.INSTANCE_NAME, props.getProperty(ConfigKeys.NAME) != null ? props.getProperty(ConfigKeys.NAME) : String.format("INSTANCE_NAME_%1$s", RandomUtils.nextInt())); Line 195: entry.context.put(ContextKeys.PROVIDES, props.getProperty(ConfigKeys.PROVIDES) != null ? props.getProperty(ConfigKeys.PROVIDES) : ""); set the logger as well: .mput(MyContextKeys.LOGGER_DUMP, dumpLogger) and set the sensitive keys from config into context. Line 196: Line 197: entry.extension = (Extension) lookupService( Line 198: Extension.class, Line 199: entry.getConfig().getProperty(ConfigKeys.CLASS), Line 197: entry.extension = (Extension) lookupService( Line 198: Extension.class, Line 199: entry.getConfig().getProperty(ConfigKeys.CLASS), Line 200: entry.getConfig().getProperty(ConfigKeys.MODULE) Line 201: ).newInstance(); I think that better to move this above, so if it fails we do not create context... not that important. Line 202: Line 203: ExtMap input = new ExtMap().mput( Line 204: Base.InvokeKeys.COMMAND, Line 205: Base.InvokeCommands.INITIALIZE Line 206: ).mput( Line 207: Base.InvokeKeys.CONTEXT, Line 208: entry.context Line 209: ); Line 210: ExtMap output = new ExtMap(); from here VVVVVV Line 211: dumpMap(entry, input); Line 212: try { Line 213: entry.extension.invoke(input, output); Line 214: } catch (Exception ex) { Line 207: Base.InvokeKeys.CONTEXT, Line 208: entry.context Line 209: ); Line 210: ExtMap output = new ExtMap(); Line 211: dumpMap(entry, input); dumpMap should get extension as this what you have from now on, the logger should be gotten out of the context. after that you need to dump the config into the log, exclude the sensitive keys, now that extension had the chance to set more of these. Line 212: try { Line 213: entry.extension.invoke(input, output); Line 214: } catch (Exception ex) { Line 215: output.mput( Line 227: entry.context.get(Base.ContextKeys.EXTENSION_NAME), Line 228: entry.context.get(Base.ContextKeys.INSTANCE_NAME) Line 229: ) Line 230: ); Line 231: dumpMap(entry, output); to here ^^^^^^^^ should be invoke() within the extension manager, to be usable by other components. every component that wants to invoke should use the same sequence so we have central logging. Line 232: int result = output.<Integer> get(Base.InvokeKeys.RESULT); Line 233: switch (result) { Line 234: case Base.InvokeResult.SUCCESS: Line 235: break; Line 232: int result = output.<Integer> get(Base.InvokeKeys.RESULT); Line 233: switch (result) { Line 234: case Base.InvokeResult.SUCCESS: Line 235: break; Line 236: case Base.InvokeResult.FAILED: this should also fail: case Base.InvokeResult.UNSUPPRTED: Line 237: throw new RuntimeException(output.<String> get(Base.InvokeKeys.MESSAGE)); Line 238: } Line 239: Line 240: entry.activated = true; Line 235: break; Line 236: case Base.InvokeResult.FAILED: Line 237: throw new RuntimeException(output.<String> get(Base.InvokeKeys.MESSAGE)); Line 238: } Line 239: construct a new logger and replace it in context. Line 240: entry.activated = true; Line 241: Line 242: } catch (Exception ex) { Line 243: log.error( Line 296: } Line 297: return serviceClass; Line 298: } Line 299: Line 300: private void dumpMap(ExtensionEntry entry, ExtMap map) { we should get a string for description... so we have something similar to the following in log: invoke: input: invoke: output: notice the "input", "output", invoke labels. Line 301: if (entry.logger.isDebugEnabled()) { Line 302: entry.logger.debug(map.toString()); Line 303: } Line 304: } -- To view, visit http://gerrit.ovirt.org/26427 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7d170d5dda990fd85e9843ecbb4909749a88df75 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
