Petals Distribution

Remove dependencies to Saxon and to easycommons-utils from petals-log

Details

  • Type: Task Task
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 4.3.0-beta-1
  • Fix Version/s: 5.0.0-M1
  • Component/s: Tools
  • Security Level: Public
  • Description:
    Hide

    petals-log depends on easycommons-utils which have a static pool of Transformers used all across petals and the components.

    petals-log is present in the system classloader so the same pool is used everywhere even though components could desire to use another transformer implementation than the default one (Saxon).

    Hence, we should remove the dependency to easycommons-utils and Saxon-HE from petals-log.

    To replace the pool, we will use for now a threadlocal Transformer.
    This means one Transformer per thread to prevent concurrent access as well as permit each thread (and thus each component) to have its desired implementation.
    This means a small impact on performance during thread creation but in components threads are reused and the transformer implementation doesn't need to change so it's ok.

    Show
    petals-log depends on easycommons-utils which have a static pool of Transformers used all across petals and the components. petals-log is present in the system classloader so the same pool is used everywhere even though components could desire to use another transformer implementation than the default one (Saxon). Hence, we should remove the dependency to easycommons-utils and Saxon-HE from petals-log. To replace the pool, we will use for now a threadlocal Transformer. This means one Transformer per thread to prevent concurrent access as well as permit each thread (and thus each component) to have its desired implementation. This means a small impact on performance during thread creation but in components threads are reused and the transformer implementation doesn't need to change so it's ok.
  • Environment:
    -

Issue Links

Activity

Hide
Victor NOËL added a comment - Fri, 3 Jul 2015 - 16:45:43 +0200

It's better to use a pool of transformers in petals-log

Show
Victor NOËL added a comment - Fri, 3 Jul 2015 - 16:45:43 +0200 It's better to use a pool of transformers in petals-log
Hide
Victor NOËL added a comment - Mon, 6 Jul 2015 - 16:44:38 +0200

Resolved for real: the kernel, just before setting up the logging system, will inject in PetalsPayloadDumperFileHandler (the handler that needed to access the Transformers pool before) a helper that delegate to the kernels classloader's SourceHelper and thus Transformers pool.

No more code duplication for SourceHandler and a shared Transformer pool with the kernel, which makes sense since the logging system is kind of part of the kernel.
The only reason we have to do that is that LogManager, the thing that instantiate the Handler, needs them to be instantiated in the system classloader, for security reasons mostly.

Show
Victor NOËL added a comment - Mon, 6 Jul 2015 - 16:44:38 +0200 Resolved for real: the kernel, just before setting up the logging system, will inject in PetalsPayloadDumperFileHandler (the handler that needed to access the Transformers pool before) a helper that delegate to the kernels classloader's SourceHelper and thus Transformers pool. No more code duplication for SourceHandler and a shared Transformer pool with the kernel, which makes sense since the logging system is kind of part of the kernel. The only reason we have to do that is that LogManager, the thing that instantiate the Handler, needs them to be instantiated in the system classloader, for security reasons mostly.
Hide
Victor NOËL added a comment - Tue, 7 Jul 2015 - 12:09:26 +0200 - edited

EDIT: ALL OF THIS IS ABANDONED, SEE NEXT COMMENT

So, that's not good enough: we should move petals-log to the kernel classloader and remove it from the system classloader where only petals-commons-log would stay (amongst other things).

This means providing our own subclass of LogManager responsible of:

  • Doing what org.ow2.petals.microkernel.server.PetalsServerImpl.initializeLogging() and org.ow2.petals.launcher.PetalsLauncher.setLogingSystemConfigurationFile(Configuration) are currently doing.
  • Loading the handlers from within the kernel classloader and not the system classloader as currently done by LogManager.

But also we should support adding extra handlers, such as petals-log-mongodb, with an easy procedure:

  • Adding the jar to the lib or extension or whatever directory
  • Configuring the logging.properties file with the handler
Show
Victor NOËL added a comment - Tue, 7 Jul 2015 - 12:09:26 +0200 - edited EDIT: ALL OF THIS IS ABANDONED, SEE NEXT COMMENT So, that's not good enough: we should move petals-log to the kernel classloader and remove it from the system classloader where only petals-commons-log would stay (amongst other things). This means providing our own subclass of LogManager responsible of:
  • Doing what org.ow2.petals.microkernel.server.PetalsServerImpl.initializeLogging() and org.ow2.petals.launcher.PetalsLauncher.setLogingSystemConfigurationFile(Configuration) are currently doing.
  • Loading the handlers from within the kernel classloader and not the system classloader as currently done by LogManager.
But also we should support adding extra handlers, such as petals-log-mongodb, with an easy procedure:
  • Adding the jar to the lib or extension or whatever directory
  • Configuring the logging.properties file with the handler
Hide
Victor NOËL added a comment - Wed, 8 Jul 2015 - 11:31:04 +0200

Ok, actually this is wrong, we go back to the first idea: having one pool for the logger handler and keeping petals-log in the system classloader: it's too much complex to change this constraint (it would mean entering in the complexity of the LogManager).

This could change in the future when switching to something like slf4j (see PETALSDISTRIB-135).

Show
Victor NOËL added a comment - Wed, 8 Jul 2015 - 11:31:04 +0200 Ok, actually this is wrong, we go back to the first idea: having one pool for the logger handler and keeping petals-log in the system classloader: it's too much complex to change this constraint (it would mean entering in the complexity of the LogManager). This could change in the future when switching to something like slf4j (see PETALSDISTRIB-135).
Hide
Victor NOËL added a comment - Wed, 8 Jul 2015 - 14:09:36 +0200

Commited, now we just need to add configurability through the handler's configuration.

Show
Victor NOËL added a comment - Wed, 8 Jul 2015 - 14:09:36 +0200 Commited, now we just need to add configurability through the handler's configuration.

People

Dates

  • Created:
    Fri, 3 Jul 2015 - 13:30:56 +0200
    Updated:
    Tue, 6 Oct 2015 - 11:12:39 +0200
    Resolved:
    Fri, 10 Jul 2015 - 13:39:51 +0200