felix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pierre De Rop (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (FELIX-5177) Support injecting configuration proxies
Date Fri, 05 Feb 2016 06:47:39 GMT

    [ https://issues.apache.org/jira/browse/FELIX-5177?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15133254#comment-15133254
] 

Pierre De Rop edited comment on FELIX-5177 at 2/5/16 6:46 AM:
--------------------------------------------------------------

I committed a slightly modified version in revision 1728564, because there was an issue regarding
how the callback was invoked.
(I also clarified the javadoc and added some comments in the source).

So, the problem was the following: so far, we had three "setCallback" methods:

- setCallback(String method): the callback is invoked on an instantiated component, and  when
the ConfigurationDependency is started, then at this point we have to instantiate the component,
because when the CM "update" thread calls our internal ConfigurationDependency.updated() callback,
then at this point we have to synchronously delegate the call to the real component instance,
and re-throw to CM any thrown exceptions.

- setCallbacks(Object callbackInstance, String callback): The callback is invoked on the callbackInstance,
but at this point, the component is not instantiated. This is typically used in a use case
where the callbackInstance acts as a Factory for the component, so the Factory is called in
the updated callback, then it can store the configuration, and later, when the component is
instantiated, then the Factory.create() method is then called, and can use the already injected
configuration before instantiating the component instance(s).

- setCallbacks(Object callbackInstance, String callback, boolean needsInstance): Here, you
explicitly specify whether or not you want an instantiated component at the time the callback
is invoked on the  callbackInstance.

That being said, now there was a problem in your setCallback(Object instance, String callback,
Class<?> configType) method:

{code}
    public ConfigurationDependency setCallback(Object instance, String callback, Class<?>
configType) {
        setCallback(instance, callback, false);
        m_configType = configType;
        return this;
    }
{code}

This is correct: since the signature contains a callbackInstance, then we don't needs the
component instance.
But the problem is that the itegration test was calling "setCallback(null, "updated", MyConfig.class)".
So, in the case, since 
no callback instance we specified, then in this case the last parameter flag (needsInstance)
should be "true", and not "false".

So, I reworked the proposed patch in order to check if the callbackInstance was null.

Now, in the ConfigurationDependencyTest, there was a small issue: the ConfigurationConsumerWithTypeSafeConfiguration
class was ensuring that "init" was called before "updated":

{code}
    static class ConfigurationConsumerWithTypeSafeConfiguration {
        private final Ensure m_ensure;

        public ConfigurationConsumerWithTypeSafeConfiguration(Ensure e) {
            m_ensure = e;
        }

        public void updated(Component component, MyConfig cfg) throws ConfigurationException
{
            Assert.assertNotNull(component);
            Assert.assertNotNull(cfg);
            m_ensure.step(3);
            if (!"testvalue".equals(cfg.getTestkey())) {
                Assert.fail("Could not find the configured property.");
            }
        }

        public void init() {
            m_ensure.step(2);
        }
    }
{code}

but this is not correct: the "updated" method must be called first, then the init() method
must be called.
So, in the init(), we should call "step(3) and in the updated(), we should call step(2).

I also introduced two more following signatures in the API:

{code}
    ConfigurationDependency setCallback(String callback, Class<?> configType);
    ConfigurationDependency setCallback(Object instance, String callback, Class<?> configType,
boolean needsInstance);
{code}

Indeed, I need these kind of signatures, specially the last one in the context of dependency
manager lambda.


So, now, the next steps are the following:

- I will rework all the samples in order to replace the usage of the bndlib "Configurable"
using the new type-safe setCallbacks method

- the Factory Configuration Pid Adapter service should also be updated in order to support
new type-safe callback.

- I also have to update the documentation.

- I will also add support for type-safe setCallbacks in the dm-lambda library, and will update
the samples with some method references, instead of the method callbacks

- I will finally modify the DM annotations in order to allow to apply a @ConfigurationDependency
on the new callback method signatures.

in the end, this patch is a great improvement !


was (Author: pderop):
I committed a slightly modified version in revision 1728564, because there was an issue regarding
how the callback was invoked.
(I also clarified the javadoc and added some comments in the source).

So, the problem was the following: so far, we had three "setCallback" methods:

- setCallback(String method): the callback is invoked on an instantiated component, and  when
the CM "update" thread calls the ConfigurationDependencyImpl.updated() method, at this point,
we have to instantiate the component because we have to invoke the updated callback on the
component instance synchronously, and re-throw exceptions (if any) to CM.

- setCallbacks(Object callbackInstance, String callback): The callback is invoked on the callbackInstance,
but at this point, the component is not instantiated. This is typically used in a use case
where the callbackInstance acts as a Factory for the component, so the Factory is called in
the updated callback, then it can store the configuration, and later, when the component is
instantiated, then the Factory.create() method is then called, and can use the already injected
configuration before instantiating the component instance(s).

- setCallbacks(Object callbackInstance, String callback, boolean needsInstance): Here, you
explicitly specify whether or not you want an instantiated component at the time the callback
is invoked on the  callbackInstance.

That being said, now there was a problem in your setCallback(Object instance, String callback,
Class<?> configType) method:

{code}
    public ConfigurationDependency setCallback(Object instance, String callback, Class<?>
configType) {
        setCallback(instance, callback, false);
        m_configType = configType;
        return this;
    }
{code}

This is correct: since the signature contains a callbackInstance, then we don't needs the
component instance.
But the problem is that the itegration test was calling "setCallback(null, "updated", MyConfig.class)".
So, in the case, since 
no callback instance we specified, then in this case the last parameter flag (needsInstance)
should be "true", and not "false".

So, I reworked the proposed patch in order to check if the callbackInstance was null.

Now, in the ConfigurationDependencyTest, there was a small issue: the ConfigurationConsumerWithTypeSafeConfiguration
class was ensuring that "init" was called before "updated":

{code}
    static class ConfigurationConsumerWithTypeSafeConfiguration {
        private final Ensure m_ensure;

        public ConfigurationConsumerWithTypeSafeConfiguration(Ensure e) {
            m_ensure = e;
        }

        public void updated(Component component, MyConfig cfg) throws ConfigurationException
{
            Assert.assertNotNull(component);
            Assert.assertNotNull(cfg);
            m_ensure.step(3);
            if (!"testvalue".equals(cfg.getTestkey())) {
                Assert.fail("Could not find the configured property.");
            }
        }

        public void init() {
            m_ensure.step(2);
        }
    }
{code}

but this is not correct: the "updated" method must be called first, then the init() method
must be called.
So, in the init(), we should call "step(3) and in the updated(), we should call step(2).

I also introduced two more following signatures in the API:

{code}
    ConfigurationDependency setCallback(String callback, Class<?> configType);
    ConfigurationDependency setCallback(Object instance, String callback, Class<?> configType,
boolean needsInstance);
{code}

Indeed, I need these kind of signatures, specially the last one in the context of dependency
manager lambda.


So, now, the next steps are the following:

- I will rework all the samples in order to replace the usage of the bndlib "Configurable"
using the new type-safe setCallbacks method

- the Factory Configuration Pid Adapter service should also be updated in order to support
new type-safe callback.

- I also have to update the documentation.

- I will also add support for type-safe setCallbacks in the dm-lambda library, and will update
the samples with some method references, instead of the method callbacks

- I will finally modify the DM annotations in order to allow to apply a @ConfigurationDependency
on the new callback method signatures.

in the end, this patch is a great improvement !

> Support injecting configuration proxies
> ---------------------------------------
>
>                 Key: FELIX-5177
>                 URL: https://issues.apache.org/jira/browse/FELIX-5177
>             Project: Felix
>          Issue Type: Improvement
>          Components: Dependency Manager, Dependency Manager Annotations, Dependency Manager
Lambda, Dependency Manager Runtime
>            Reporter: J.W. Janssen
>            Assignee: Pierre De Rop
>             Fix For: 	org.apache.felix.dependencymanager-r7
>
>         Attachments: FELIX-5177.patch
>
>
> DM supports mandatory configurations, but does not allow anything other than a dictionary
to be passed to the callback. In other DI frameworks (like DS) it is possible to use type-safe
configurations and let those be injected instead of plain dictionaries.
> It would be great if DM also would support this, as it would remove lots of configuration
boiler plate code from our projects.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message