From dev-return-35421-apmail-directory-dev-archive=directory.apache.org@directory.apache.org Wed Oct 27 13:07:47 2010 Return-Path: Delivered-To: apmail-directory-dev-archive@www.apache.org Received: (qmail 17433 invoked from network); 27 Oct 2010 13:07:47 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 27 Oct 2010 13:07:47 -0000 Received: (qmail 96140 invoked by uid 500); 27 Oct 2010 13:07:47 -0000 Delivered-To: apmail-directory-dev-archive@directory.apache.org Received: (qmail 96083 invoked by uid 500); 27 Oct 2010 13:07:46 -0000 Mailing-List: contact dev-help@directory.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: "Apache Directory Developers List" Delivered-To: mailing list dev@directory.apache.org Received: (qmail 96066 invoked by uid 99); 27 Oct 2010 13:07:46 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 27 Oct 2010 13:07:46 +0000 X-ASF-Spam-Status: No, hits=-1997.8 required=10.0 tests=ALL_TRUSTED,HTML_MESSAGE X-Spam-Check-By: apache.org Received: from [140.211.11.40] (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 27 Oct 2010 13:07:43 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (8.14.3/8.14.3/Debian-9.1ubuntu1) with ESMTP id o9RD7hwR008644; Wed, 27 Oct 2010 13:07:43 GMT Content-Type: multipart/alternative; boundary="===============8062023984113549473==" MIME-Version: 1.0 Subject: Re: Review Request: ApacheDsService.start() method refactoring From: =?utf-8?q?Emmanuel_L=C3=A9charny?= To: =?utf-8?q?directory?= , =?utf-8?q?Kiran_Ayyagari?= , akarasulu@apache.org, =?utf-8?q?Emmanuel_L=C3=A9charny?= Date: Wed, 27 Oct 2010 13:07:43 -0000 Message-ID: <20101027130743.27681.2575@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org X-ReviewRequest-URL: https://reviews.apache.org/r/14/ In-Reply-To: <20101027113820.27681.35858@reviews.apache.org> References: <20101027113820.27681.35858@reviews.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org --===============8062023984113549473== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On 2010-10-27 04:38:22, Kiran Ayyagari wrote: > > I would suggest that we confine the usage of beans to config reader onl= y(even if they can be accessed publicly), cause we need to copy the propert= y values from bean to the corresponding real instance (e.x DirectoryService= Bean -> DirectoryService) and this is better to it in the reader, so we don= 't need this copying anywhere else to setup a service (one particular area = I see is in embedded mode, currently in embedded server we are not able to > > use external config, but with the new single file LDIF based config it = is now possible and setting up a server gets easier too from a user's pov) > = > Emmanuel L=C3=A9charny wrote: > I'm not sure this is a good idea : > - if the configBean does the Bean -> real object conversion, then the= configBean will have to be aware of the real object. In any case, there is= a tight relation between config and entities, and it has to be expressed i= n either place. > - As configBean can be used to store partial results (it contains Ads= BaseBeans), you have no way to know which kind of objects you'll get from i= t : ot can stores AdirectoryService beans (and more than one), Servers, etc. > - if the service init needs to read the configBean and search for the= exact Bean it has to use, then that mean the service has to know where in = the config hierarchy will be stored the Bean. Not very convenient, IMO > - also the reader reads, it does not instantiate. In Studio, we don't= care instantiating anything, so there is no reason to have this part done = in the reader. I created a ConfigCreator for this purpose, btw > = > Kiran Ayyagari wrote: > Agree with you, the main thing I want to see is to have a single plav= e where this bean -> real service object conversion/mapping/creation = > happens. > So instead of we changing the startLdap() method to startLdap( direct= oryServiceBean.getLdapServerBean(), directoryService ); > = > we need a config handler/creator to do this and give us the Directory= Service which we can use directly to start. Am I making sense? > > = > Emmanuel L=C3=A9charny wrote: > ATM, the ConfigCreator class is doing the conversion. = > = > We can do : startLdap( directoryServiceBean, directoryService ) > = > that would be probably better. > = > If we go any further, another approach would be to nae all the beans,= and use PicoContainer : > = > startLdap( container, directoryService ) > = > where inside the startLdap we would do : > = > LdapServerBean ldapServerBean (LdapServerBean)container.get( "ldapS= erver" ); > = > but this is not where we want to go, I guess... > = > > = > Kiran Ayyagari wrote: > >> We can do : startLdap( directoryServiceBean, directoryService ) > IMHO I want this to be done in the config creator, i.e. like startLda= p ( configCreator.createDS(directoryServiceBean) ); > so if I want to embed the DS I can use config reader and creator and = just start the DS like > DirectoryService ds =3D configCreator.createDS(directoryServiceBean); > ds.startup(); > = > I need not copy the code from startLdap( directoryServiceBean, direct= oryService ) to create the DS and then start > = > >> but this is not where we want to go, I guess... > definitely NOT :) > > = > akarasulu wrote: > +1 on ds.startup() after configCreator.createDS(directoryServiceBean) > this makes for a cleaner API. > = > Emmanuel L=C3=A9charny wrote: > ok, so what about creating everything in the bean builder, and pass t= he result to the start methods ? But again, we would like to write : > = > startLdap( ldapInstance, dsInstance ), no ? > = > And how do we get the ldapInstance, btw? configBuilder.getLdapInstanc= e() ? > = > This won't be different from what we currently have... > = > akarasulu wrote: > elechary clarified the 3 steps in the startup process on IRC: > = > 1). Read the configuration from LDAP configuration partition and buil= d the configuration bean hierarchy > 2). Using the configuration bean hierarchy build a DirectoryService o= r other ApacheDSService instance > 3). Start the instance for the DirectoryService or the ApacheDSService > = > My 2 cents say keep this all intuitive and easily inferred by our API= users keeping the amount of manual reading they have to do to just get the= job done to a minimum. That means a clean API. Clean API =3D less methods,= less overloads, less arguments, less classes and a clear process with clea= r intuitive names. Here's some recommendations for the builders working at = step 1 and 2. > = > 1). Since we're building a containment tree of configuration objects = like someone would a DOM I would associate this with the builder pattern an= d name the base class that does this ConfigurationBeanBuilder. > 2). Since we're building a containment tree of components I would ass= ociate this again with the builder pattern and name the class that does thi= s DirectoryServiceBuilder. > sorry, I think I see now what you'd like to have : ds =3D beanBuilder.createDS( dsBean ); ds.startup(); ldapServer =3D beanBuilder.createLdapServer( dsBean ); ldapServer.startup(); ... Is that it ? - Emmanuel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14/#review5 ----------------------------------------------------------- On 2010-10-27 04:24:41, Emmanuel L=C3=A9charny wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14/ > ----------------------------------------------------------- > = > (Updated 2010-10-27 04:24:41) > = > = > Review request for directory. > = > = > Summary > ------- > = > I have modified the ApacheDsService start method this way : > = > public void start( InstanceLayout instanceLayout ) throws Exception > { > File partitionsDir =3D instanceLayout.getPartitionsDirectory(); > = > if ( !partitionsDir.exists() ) > { > LOG.info( "partition directory doesn't exist, creating {}", p= artitionsDir.getAbsolutePath() ); > partitionsDir.mkdirs(); > } > = > LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath= () ); > = > initSchemaLdifPartition( instanceLayout ); > initConfigPartition( instanceLayout ); > = > // Read the configuration > cpReader =3D new ConfigPartitionReader( configPartition, partitio= nsDir ); > = > ConfigBean configBean =3D cpReader.readConfig( "ou=3Dconfig" ); > = > DirectoryServiceBean directoryServiceBean =3D configBean.getDirec= toryServiceBean( "default" ); > = > // Initialize the DirectoryService now > DirectoryService directoryService =3D initDirectoryService( insta= nceLayout, directoryServiceBean ); > = > // start the LDAP server > startLdap( directoryServiceBean.getLdapServerBean(), directorySer= vice ); > = > // start the NTP server > startNtp( directoryServiceBean.getNtpServerBean(), directoryServi= ce ); > = > // Initialize the DNS server (Not ready yet) > // initDns( configBean ); > = > // Initialize the DHCP server (Not ready yet) > // initDhcp( configBean ); > = > // start the ChangePwd server (Not ready yet) > startChangePwd( directoryServiceBean.getChangePasswordServerBean(= ), directoryService ); > = > // start the Kerberos server > startKerberos( directoryServiceBean.getKdcServerBean(), directory= Service ); > = > // start the jetty http server > startHttpServer( directoryServiceBean.getHttpServerBean(), direct= oryService ); > } > = > = > Diffs > ----- > = > = > Diff: https://reviews.apache.org/r/14/diff > = > = > Testing > ------- > = > = > Thanks, > = > Emmanuel > = > --===============8062023984113549473== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: https://reviews.apache.o= rg/r/14/

On October 27th, 2010, 4:38 a.m., Kiran Ayy= agari wrote:

I would s=
uggest that we confine the usage of beans to config reader only(even if the=
y can be accessed publicly), cause we need to copy the property values from=
 bean to the corresponding real instance (e.x DirectoryServiceBean -> Di=
rectoryService) and this is better to it in the reader, so we don't nee=
d this copying anywhere else to setup a service (one particular area I see =
is in embedded mode, currently in embedded server we are not able to
use external config, but with the new single file LDIF based config it is n=
ow possible and setting up a server gets easier too from a user's pov)<=
/pre>

On October 27th, 2010, 4:52 a.m., Emmanuel L=C3=A9charny wrote:<= /p> 

I'm n=
ot sure this is a good idea :
- if the configBean does the Bean -> real object conversion, then the co=
nfigBean will have to be aware of the real object. In any case, there is a =
tight relation between config and entities, and it has to be expressed in e=
ither place.
- As configBean can be used to store partial results (it contains AdsBaseBe=
ans), you have no way to know which kind of objects you'll get from it =
: ot can stores AdirectoryService beans (and more than one), Servers, etc.
- if the service init needs to read the configBean and search for the exact=
 Bean it has to use, then that mean the service has to know where in the co=
nfig hierarchy will be stored the Bean. Not very convenient, IMO
- also the reader reads, it does not instantiate. In Studio, we don't c=
are instantiating anything, so there is no reason to have this part done in=
 the reader. I created a ConfigCreator for this purpose, btw

On October 27th, 2010, 5:20 a.m., Kiran Ayyagari wrote:

Agree wit=
h you, the main thing I want to see is to have a single plave where this be=
an -> real service object conversion/mapping/creation =

happens.
So instead of we changing the startLdap() method to startLdap( directorySer=
viceBean.getLdapServerBean(), directoryService );

we need a config handler/creator to do this and give us the DirectoryServic=
e which we can use directly to start. Am I making sense?

On October 27th, 2010, 5:37 a.m., Emmanuel L=C3=A9charny wrote:<= /p> 

ATM, the =
ConfigCreator class is doing the conversion. =


We can do : startLdap( directoryServiceBean, directoryService )

that would be probably better.

If we go any further, another approach would be to nae all the beans, and u=
se PicoContainer :

startLdap( container, directoryService )

where inside the startLdap we would do :

  LdapServerBean ldapServerBean (LdapServerBean)container.get( "ldapSe=
rver" );

but this is not where we want to go, I guess...

On October 27th, 2010, 5:43 a.m., Kiran Ayyagari wrote:

>> =
We can do : startLdap( directoryServiceBean, directoryService )
IMHO I want this to be done in the config creator, i.e. like startLdap ( co=
nfigCreator.createDS(directoryServiceBean) );
so if I want to embed the DS I can use config reader and creator and just s=
tart the DS like
DirectoryService ds =3D configCreator.createDS(directoryServiceBean);
ds.startup();

I need not copy the code from startLdap( directoryServiceBean, directorySer=
vice ) to create the DS and then start

>> but this is not where we want to go, I guess...
definitely NOT :)

On October 27th, 2010, 6 a.m., akarasulu wrote:

+1 on ds.=
startup() after configCreator.createDS(directoryServiceBean)
 this makes for a cleaner API.

On October 27th, 2010, 6:06 a.m., Emmanuel L=C3=A9charny wrote:<= /p> 

ok, so wh=
at about creating everything in the bean builder, and pass the result to th=
e start methods ? But again, we would like to write :

startLdap( ldapInstance, dsInstance ), no ?

And how do we get the ldapInstance, btw? configBuilder.getLdapInstance() ?

This won't be different from what we currently have...

On October 27th, 2010, 6:07 a.m., akarasulu wrote:

elechary =
clarified the 3 steps in the startup process on IRC:

1). Read the configuration from LDAP configuration partition and build the =
configuration bean hierarchy
2). Using the configuration bean hierarchy build a DirectoryService or othe=
r ApacheDSService instance
3). Start the instance for the DirectoryService or the ApacheDSService

My 2 cents say keep this all intuitive and easily inferred by our API users=
 keeping the amount of manual reading they have to do to just get the job d=
one to a minimum. That means a clean API. Clean API =3D less methods, less =
overloads, less arguments, less classes and a clear process with clear intu=
itive names. Here's some recommendations for the builders working at st=
ep 1 and 2.

1). Since we're building a containment tree of configuration objects li=
ke someone would a DOM I would associate this with the builder pattern and =
name the base class that does this ConfigurationBeanBuilder.
2). Since we're building a containment tree of components I would assoc=
iate this again with the builder pattern and name the class that does this =
DirectoryServiceBuilder.
 
sorry, I th=
ink I see now what you'd like to have :

ds =3D beanBuilder.createDS( dsBean );
ds.startup();

ldapServer =3D beanBuilder.createLdapServer( dsBean );
ldapServer.startup();

...

Is that it ?

- Emmanuel


On October 27th, 2010, 4:24 a.m., Emmanuel L=C3=A9charny wrote:

Review request for directory.
By Emmanuel L=C3=A9charny.

Updated 2010-10-27 04:24:41

Descripti= on 

I have modified the ApacheDsService start method this way :

    public void start( InstanceLayout instanceLayout ) throws Exception
    {
        File partitionsDir =3D instanceLayout.getPartitionsDirectory();
        =

        if ( !partitionsDir.exists() )
        {
            LOG.info( "partition directory doesn't exist, creating=
 {}", partitionsDir.getAbsolutePath() );
            partitionsDir.mkdirs();
        }

        LOG.info( "using partition dir {}", partitionsDir.getAbso=
lutePath() );
        =

        initSchemaLdifPartition( instanceLayout );
        initConfigPartition( instanceLayout );

        // Read the configuration
        cpReader =3D new ConfigPartitionReader( configPartition, partitions=
Dir );
        =

        ConfigBean configBean =3D cpReader.readConfig( "ou=3Dconfig&qu=
ot; );
        =

        DirectoryServiceBean directoryServiceBean =3D configBean.getDirecto=
ryServiceBean( "default" );
        =

        // Initialize the DirectoryService now
        DirectoryService directoryService =3D initDirectoryService( instanc=
eLayout, directoryServiceBean );

        // start the LDAP server
        startLdap( directoryServiceBean.getLdapServerBean(), directoryServi=
ce );

        // start the NTP server
        startNtp( directoryServiceBean.getNtpServerBean(), directoryService=
 );

        // Initialize the DNS server (Not ready yet)
        // initDns( configBean );

        // Initialize the DHCP server (Not ready yet)
        // initDhcp( configBean );

        // start the ChangePwd server (Not ready yet)
        startChangePwd( directoryServiceBean.getChangePasswordServerBean(),=
 directoryService );

        // start the Kerberos server
        startKerberos( directoryServiceBean.getKdcServerBean(), directorySe=
rvice );

        // start the jetty http server
        startHttpServer( directoryServiceBean.getHttpServerBean(), director=
yService );
    }

Diffs=

View Diff
--===============8062023984113549473==--