joerghoh / cq5-healthcheck

CQ5 Healthcheck code
Apache License 2.0
28 stars 15 forks source link

Use OSGI configs instead of custom config for status providers #13

Closed alexsaar closed 11 years ago

alexsaar commented 11 years ago

Similar to setting up new loggers. This would allow to

mhaack commented 11 years ago

+1

alexsaar commented 11 years ago

implemented a first working prototype that shows how this can look like and pushed the changes to joerghoh/cq5-healthcheck/tree/13-OSGIConfig.

in addition to the already mentioned benefits the approach saves a lot of lines as we don't need the factory anymore that listenens to repository events and manages services.

let me know what you think.

joerghoh commented 11 years ago
alexsaar commented 11 years ago

it basically works the same as before from a usage pov. but instead of adding sling:Folder to /etc/healthcheck/mbeans you can add nodes of type sling:OsgiConfig to your application. So we are using standardized configuration interfaces and make use of the frameworks mechanisms.

for this I removed the custom service factory and use SCR annotations to register new instances ofMBeanStatusProvider.

This is done by using

@Component(label = "MBean Status Provider Factory", metatype = true, configurationFactory = true, policy = ConfigurationPolicy.REQUIRE)

everything else stays the same and all the functionality is still there.

mhaack commented 11 years ago

As a small enhancement I would propose to add some additional check in the active method to check:

a) if the MBean does not exits throw an exception b) initial check of the MBean attribute comparison and throw an exception if the configured comparison expression is invalid c) allow to configure multiple attributes to be checked per MBean

What do you think about that?

mhaack commented 11 years ago

Ignore c) I just saw mbean.property is already a String[]

alexsaar commented 11 years ago

agreed on a) and b). I'd also move the property parsing to activate method. no need to do this on every getStatus.

regarding c) while we have this option on the config side, from an API pov we still need one instance per criteria. but it should be easily doable to implement this with the new API from #15 that allows for nested states.