[Fortress] Manager instanciations

[Emmanuel Lécharny <el...@gmail.com>]

Hi guys,

looking at the way we use the managers, we do create a new instance of
each one of them everytime we need to execute some operation.

Right now, we are creating an instance based on some code like :

    public static DelAdminMgr createInstance(String contextId) throws
SecurityException
    {
        if ( Strings.isEmpty( dAdminClassName ) )
        {
            if ( GlobalIds.IS_REST )
            {
                dAdminClassName = DelAdminMgrRestImpl.class.getName();
            }
            else
            {
                dAdminClassName = DelAdminMgrImpl.class.getName();
            }
            
            DelAdminMgr delAdminMgr = (DelAdminMgr)
ClassUtil.createInstance( dAdminClassName );
            delAdminMgr.setContextId(contextId);
        }
        ...

There are five problems in this approach :

1) Once we have determinated the type of connector, we can't switch to
any other. This is due to the first test :
 
 if ( Strings.isEmpty( dAdminClassName ) )

As soon as we have defined the class name, it's for ever, unless we set
it to null, which is not really an option.

2) It's static : we base the selection of a class by a gloabbly defined
value, IS_REST. There is no way for the user to switch this to something
different :

    public static final boolean IS_REST = ( ( Config.getProperty(
ENABLE_REST ) != null ) && ( Config
        .getProperty( ENABLE_REST ).equalsIgnoreCase( "true" ) ) );

3) It's limited. Would we want to add a third possible connector (say,
to a database), then we will have to change all the factory to add
another check like :

            if ( GlobalIds.IS_REST )
            {
                dAdminClassName = DelAdminMgrRestImpl.class.getName();
            }
            else if ( GlobalIds.IS_DB )
            {
                dAdminClassName = DelAdminMgrDbImpl.class.getName();
            }
            else
            {
                dAdminClassName = DelAdminMgrImpl.class.getName();
            }

 That requires a new version of the API.
 
 4) It's costly, as we have to call a Class.forName( className
).newInstance() every time we want an instance.
 
 5) It's not context free, as we inject a contextId into the instance.
 
 
 Now, considering those issues, we can separate them into two different
concerns :
 - the first concern is about the static side of such an operation
(points 1 to 4)
 - the second concern is about the context 'fulliness' of the approach.

Here, a container approach would be way better. A lightweight container
could do the trick. You require an instance of a class based on a config
parameter which is not static, and you get back an instance. The
container is responsible for the instanciation. Here, the container
would take two parameters : the type of manager, and the type of
connector. Something like :
    
    DelAdminMgr delAdminMgr = container.createInstance( "DelAdminMgr",
context.getConnectionType() );
 
The connection type is provided by a global context object which is
initialized before we start the API, and that could be modified on the
fly, allowing teh user to require different type of instances.

Adding a new connector is just a mater of defining a mapping between a
name (here "DelAmdinMgr") and a class to instanciate. the tuple <name,
type> would is used to retrieve teh correct class.

A plugin approach, for connectors, would require that they push the
classes to be used into the container. For instance, a DB connector
would register itself into the container by pushing the [
<"DelAmdinMgr", DB_CONNECTOR>, DelAdminDbMgr.class.getName() ] tuple
into the container.

That is one of the possible options, there are many more.

Dealing with the second aspect is a bit more complex, as it requires we
check all the code to be sure that the instance is stateless. Although
it's important to think about doing this check, and eventually move to a
stateless instance, because it will require way less instances to be
created, and we won't have any more to care about the potential memory
cost of it (even if we will have to pass a context to each instance).

In any case, if the instance is stateless, we can stop thinking about
creating a new instance for each call, we just have to return the first
instance we created. And even if they are not stateless, assuming that
the contextID is the only parameter in used, then we could manage a
cache of instance for each specific context.


Thoughts ?
 

Re: [Fortress] Manager instanciations

[Shawn McKinney <mc...@att.net>]

On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:
> looking at the way we use the managers, we do create a new instance of
> each one of them everytime we need to execute some operation.

A new instance is created the first time needed and may be cached afterwards as it is merely a POJO.

The createInstance must be able to....

1. accept a reference to session containing delegated admin context.  This is used for ARBAC02 style permission checks to occur within RBAC managers.
2. accept a tenant id.  This scopes the subsequent operations to a particular subtree within the DIT - i.e. multitenancy


If either of the above two instance varables are set on the manager, the instance will not be thread safe.  If either of the two above change in between operations, a new manager must be created to correspond with the new context.


On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:> There are five problems in this approach :
> 
> 1) Once we have determinated the type of connector, we can't switch to
> any other. This is due to the first test :
>  
>  if ( Strings.isEmpty( dAdminClassName ) )
> 
> As soon as we have defined the class name, it's for ever, unless we set
> it to null, which is not really an option.

True


On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:> 2) It's static : we base the selection of a class by a gloabbly defined
> value, IS_REST. There is no way for the user to switch this to something
> different :

Also true. Not sure of a use case where this creates a problem.  


On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:>  4) It's costly, as we have to call a Class.forName( className
> ).newInstance() every time we want an instance.
>

Again true


On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:>  
>  5) It's not context free, as we inject a contextId into the instance.

Yes there are two possible injections, refer to my earlier comments.



On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:> Here, a container approach would be way better. A lightweight container
> could do the Wtrick. You require an instance of a class based on a config
> parameter which is not static, and you get back an instance. The
> container is responsible for the instanciation. Here, the container
> would take two parameters : the type of manager, and the type of
> connector. Something like :
>     
>     DelAdminMgr delAdminMgr = container.createInstance( "DelAdminMgr",
> context.getConnectionType() );


Would work


On 10/31/2014 08:28 AM, Emmanuel Lécharny wrote:> Dealing with the second aspect is a bit more complex, as it requires we
> check all the code to be sure that the instance is stateless. Although
> it's important to think about doing this check, and eventually move to a
> stateless instance, because it will require way less instances to be
> created, and we won't have any more to care about the potential memory
> cost of it (even if we will have to pass a context to each instance).
> 
> In any case, if the instance is stateless, we can stop thinking about
> creating a new instance for each call, we just have to return the first
> instance we created. And even if they are not stateless, assuming that
> the contextID is the only parameter in used, then we could manage a
> cache of instance for each specific context.

Are you saying that the manager impl should be stateless?  What about multitenancy and ARBAC req's?  The ARBAC context is for a particular user so I don't think a cache will work here.