You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Emmanuel Lécharny <el...@apache.org> on 2010/10/27 13:24:42 UTC
Review Request: ApacheDsService.start() method refactoring
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14/
-----------------------------------------------------------
Review request for directory.
Summary
-------
I have modified the ApacheDsService start method this way :
public void start( InstanceLayout instanceLayout ) throws Exception
{
File partitionsDir = instanceLayout.getPartitionsDirectory();
if ( !partitionsDir.exists() )
{
LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
partitionsDir.mkdirs();
}
LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
initSchemaLdifPartition( instanceLayout );
initConfigPartition( instanceLayout );
// Read the configuration
cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
ConfigBean configBean = cpReader.readConfig( "ou=config" );
DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
// Initialize the DirectoryService now
DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
// start the LDAP server
startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
// 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(), directoryService );
// start the jetty http server
startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
}
Diffs
-----
Diff: https://reviews.apache.org/r/14/diff
Testing
-------
Thanks,
Emmanuel
Re: Review Request: ApacheDsService.start() method refactoring
Posted by ak...@apache.org.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> This won't be different from what we currently have...
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 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
- akarasulu
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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.
ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
This won't be different from what we currently have...
- 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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)
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 in either place.
- As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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
- 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by ak...@apache.org.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
>
> akarasulu wrote:
> NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
>
> DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
> DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
> DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
> ds.startup();
>
> akarasulu wrote:
> Yeah Emmanuel this is what I was thinking. Except I want to stop using the "Bean" suffix on classes and identifiers. Why? Because we all know they are beans. Even the components are beans though and that's ambiguous to me. Why not just say config instead and it's understood that this is just a value object (a bean) used for the configuration info.
>
> BTW one other point. Builders can have some member properties that impact the way they build. However most often we're not going to deal with any. And in this case the need to have an instance of a builder is moot I think now but who knows. So why not just have a configuration builder static method that builds the config bean hierarchy to have simpler code like so:
>
> DirectoryServiceConfig dsConfig = DirectoryServiceConfigBuilder.build( dsConfigDn );
> DirectoryService ds = DirectoryServiceBuilder.build( dsConfig );
> ds.startup();
>
> WDYT ?
>
> Emmanuel Lécharny wrote:
> We have to be able to make a distinction between DirectoryService and DirectoryService<Bean>, otherwise we have a conflict. This is true for *all* the elemnts, except for the config. Why would the ConfigBean be different ? We could thogh name it ConfigContainer...
>
> I buy the idea of having static methods. There is no need to create a Builder objetc anyway, we don't store any data into it.
Hmm ok I am not aware or did not understand this generic construct you have here can you elaborate on it?
Here's what I was thinking: You have a DirectoryService and you have a DirectoryServiceConfig. However currently we're calling the DirectoryServiceConfig a DirectoryServiceBean. Did I understand this correctly?
- akarasulu
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
>
> akarasulu wrote:
> NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
>
> DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
> DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
> DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
> ds.startup();
>
> akarasulu wrote:
> Yeah Emmanuel this is what I was thinking. Except I want to stop using the "Bean" suffix on classes and identifiers. Why? Because we all know they are beans. Even the components are beans though and that's ambiguous to me. Why not just say config instead and it's understood that this is just a value object (a bean) used for the configuration info.
>
> BTW one other point. Builders can have some member properties that impact the way they build. However most often we're not going to deal with any. And in this case the need to have an instance of a builder is moot I think now but who knows. So why not just have a configuration builder static method that builds the config bean hierarchy to have simpler code like so:
>
> DirectoryServiceConfig dsConfig = DirectoryServiceConfigBuilder.build( dsConfigDn );
> DirectoryService ds = DirectoryServiceBuilder.build( dsConfig );
> ds.startup();
>
> WDYT ?
>
> Emmanuel Lécharny wrote:
> We have to be able to make a distinction between DirectoryService and DirectoryService<Bean>, otherwise we have a conflict. This is true for *all* the elemnts, except for the config. Why would the ConfigBean be different ? We could thogh name it ConfigContainer...
>
> I buy the idea of having static methods. There is no need to create a Builder objetc anyway, we don't store any data into it.
>
> akarasulu wrote:
> Hmm ok I am not aware or did not understand this generic construct you have here can you elaborate on it?
>
> Here's what I was thinking: You have a DirectoryService and you have a DirectoryServiceConfig. However currently we're calling the DirectoryServiceConfig a DirectoryServiceBean. Did I understand this correctly?
>
> Emmanuel Lécharny wrote:
> That's it.
>
> Kiran Ayyagari wrote:
> Emmanuel, yes that is what I was expecting, thanks.
Let me draft a new version, as soon as I'm done with the ChangePassword creation.
- 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
sorry, I think I see now what you'd like to have :
ds = beanBuilder.createDS( dsBean );
ds.startup();
ldapServer = 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Kiran Ayyagari <ka...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
>
> akarasulu wrote:
> NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
>
> DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
> DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
> DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
> ds.startup();
>
> akarasulu wrote:
> Yeah Emmanuel this is what I was thinking. Except I want to stop using the "Bean" suffix on classes and identifiers. Why? Because we all know they are beans. Even the components are beans though and that's ambiguous to me. Why not just say config instead and it's understood that this is just a value object (a bean) used for the configuration info.
>
> BTW one other point. Builders can have some member properties that impact the way they build. However most often we're not going to deal with any. And in this case the need to have an instance of a builder is moot I think now but who knows. So why not just have a configuration builder static method that builds the config bean hierarchy to have simpler code like so:
>
> DirectoryServiceConfig dsConfig = DirectoryServiceConfigBuilder.build( dsConfigDn );
> DirectoryService ds = DirectoryServiceBuilder.build( dsConfig );
> ds.startup();
>
> WDYT ?
>
> Emmanuel Lécharny wrote:
> We have to be able to make a distinction between DirectoryService and DirectoryService<Bean>, otherwise we have a conflict. This is true for *all* the elemnts, except for the config. Why would the ConfigBean be different ? We could thogh name it ConfigContainer...
>
> I buy the idea of having static methods. There is no need to create a Builder objetc anyway, we don't store any data into it.
>
> akarasulu wrote:
> Hmm ok I am not aware or did not understand this generic construct you have here can you elaborate on it?
>
> Here's what I was thinking: You have a DirectoryService and you have a DirectoryServiceConfig. However currently we're calling the DirectoryServiceConfig a DirectoryServiceBean. Did I understand this correctly?
>
> Emmanuel Lécharny wrote:
> That's it.
Emmanuel, yes that is what I was expecting, thanks.
- Kiran
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by ak...@apache.org.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) to create the DS and then start
>
> >> but this is not where we want to go, I guess...
> definitely NOT :)
>
+1 on ds.startup() after configCreator.createDS(directoryServiceBean)
this makes for a cleaner API.
- akarasulu
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Kiran Ayyagari <ka...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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
Agree with you, the main thing I want to see is to have a single plave where this bean -> real service object conversion/mapping/creation
happens.
So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
- Kiran
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by ak...@apache.org.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
ds.startup();
- akarasulu
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
>
> akarasulu wrote:
> NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
>
> DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
> DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
> DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
> ds.startup();
>
> akarasulu wrote:
> Yeah Emmanuel this is what I was thinking. Except I want to stop using the "Bean" suffix on classes and identifiers. Why? Because we all know they are beans. Even the components are beans though and that's ambiguous to me. Why not just say config instead and it's understood that this is just a value object (a bean) used for the configuration info.
>
> BTW one other point. Builders can have some member properties that impact the way they build. However most often we're not going to deal with any. And in this case the need to have an instance of a builder is moot I think now but who knows. So why not just have a configuration builder static method that builds the config bean hierarchy to have simpler code like so:
>
> DirectoryServiceConfig dsConfig = DirectoryServiceConfigBuilder.build( dsConfigDn );
> DirectoryService ds = DirectoryServiceBuilder.build( dsConfig );
> ds.startup();
>
> WDYT ?
We have to be able to make a distinction between DirectoryService and DirectoryService<Bean>, otherwise we have a conflict. This is true for *all* the elemnts, except for the config. Why would the ConfigBean be different ? We could thogh name it ConfigContainer...
I buy the idea of having static methods. There is no need to create a Builder objetc anyway, we don't store any data into 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Kiran Ayyagari <ka...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> but this is not where we want to go, I guess...
>
>
>> We can do : startLdap( directoryServiceBean, directoryService )
IMHO I want this to be done in the config creator, i.e. like startLdap ( 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 = configCreator.createDS(directoryServiceBean);
ds.startup();
I need not copy the code from startLdap( directoryServiceBean, directoryService ) to create the DS and then start
>> but this is not where we want to go, I guess...
definitely NOT :)
- Kiran
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
>
> akarasulu wrote:
> NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
>
> DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
> DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
> DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
> ds.startup();
>
> akarasulu wrote:
> Yeah Emmanuel this is what I was thinking. Except I want to stop using the "Bean" suffix on classes and identifiers. Why? Because we all know they are beans. Even the components are beans though and that's ambiguous to me. Why not just say config instead and it's understood that this is just a value object (a bean) used for the configuration info.
>
> BTW one other point. Builders can have some member properties that impact the way they build. However most often we're not going to deal with any. And in this case the need to have an instance of a builder is moot I think now but who knows. So why not just have a configuration builder static method that builds the config bean hierarchy to have simpler code like so:
>
> DirectoryServiceConfig dsConfig = DirectoryServiceConfigBuilder.build( dsConfigDn );
> DirectoryService ds = DirectoryServiceBuilder.build( dsConfig );
> ds.startup();
>
> WDYT ?
>
> Emmanuel Lécharny wrote:
> We have to be able to make a distinction between DirectoryService and DirectoryService<Bean>, otherwise we have a conflict. This is true for *all* the elemnts, except for the config. Why would the ConfigBean be different ? We could thogh name it ConfigContainer...
>
> I buy the idea of having static methods. There is no need to create a Builder objetc anyway, we don't store any data into it.
>
> akarasulu wrote:
> Hmm ok I am not aware or did not understand this generic construct you have here can you elaborate on it?
>
> Here's what I was thinking: You have a DirectoryService and you have a DirectoryServiceConfig. However currently we're calling the DirectoryServiceConfig a DirectoryServiceBean. Did I understand this correctly?
That's 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Emmanuel Lécharny <el...@apache.org>.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
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( "ldapServer" );
but this is not where we want to go, I guess...
- 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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by ak...@apache.org.
> On 2010-10-27 04:38:22, Kiran Ayyagari wrote:
> > I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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écharny 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 in either place.
> - As configBean can be used to store partial results (it contains AdsBaseBeans), 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 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 plave where this bean -> real service object conversion/mapping/creation
> happens.
> So instead of we changing the startLdap() method to startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> we need a config handler/creator to do this and give us the DirectoryService which we can use directly to start. Am I making sense?
>
>
> Emmanuel Lécharny 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( "ldapServer" );
>
> 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 startLdap ( 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 = configCreator.createDS(directoryServiceBean);
> ds.startup();
>
> I need not copy the code from startLdap( directoryServiceBean, directoryService ) 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écharny wrote:
> ok, so what about creating everything in the bean builder, and pass the 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.getLdapInstance() ?
>
> 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 build the configuration bean hierarchy
> 2). Using the configuration bean hierarchy build a DirectoryService or 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 = less methods, less overloads, less arguments, less classes and a clear process with clear 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 and name the base class that does this ConfigurationBeanBuilder.
> 2). Since we're building a containment tree of components I would associate this again with the builder pattern and name the class that does this DirectoryServiceBuilder.
>
>
> Emmanuel Lécharny wrote:
> sorry, I think I see now what you'd like to have :
>
> ds = beanBuilder.createDS( dsBean );
> ds.startup();
>
> ldapServer = beanBuilder.createLdapServer( dsBean );
> ldapServer.startup();
>
> ...
>
> Is that it ?
>
> akarasulu wrote:
> NOTE: so you can have code like this (WARNING: extra verbose for now to be didactic):
>
> DirectoryServiceConfigBuilder dsConfigBuilder = new DirectoryServiceConfigurationBuilder( LdapDN dsConfigDn );
> DirectoryServiceConfig dsConfig = dsConfigBuilder.buildConfig();
> DirectoryService ds = DirectoryServiceBuilder.build( buildConfig );
> ds.startup();
Yeah Emmanuel this is what I was thinking. Except I want to stop using the "Bean" suffix on classes and identifiers. Why? Because we all know they are beans. Even the components are beans though and that's ambiguous to me. Why not just say config instead and it's understood that this is just a value object (a bean) used for the configuration info.
BTW one other point. Builders can have some member properties that impact the way they build. However most often we're not going to deal with any. And in this case the need to have an instance of a builder is moot I think now but who knows. So why not just have a configuration builder static method that builds the config bean hierarchy to have simpler code like so:
DirectoryServiceConfig dsConfig = DirectoryServiceConfigBuilder.build( dsConfigDn );
DirectoryService ds = DirectoryServiceBuilder.build( dsConfig );
ds.startup();
WDYT ?
- akarasulu
-----------------------------------------------------------
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écharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>
Re: Review Request: ApacheDsService.start() method refactoring
Posted by Kiran Ayyagari <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14/#review5
-----------------------------------------------------------
Ship it!
I would suggest that we confine the usage of beans to config reader only(even if they can be accessed publicly), cause we need to copy the property values from bean to the corresponding real instance (e.x DirectoryServiceBean -> 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)
- Kiran
On 2010-10-27 04:24:41, Emmanuel Lécharny 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 = instanceLayout.getPartitionsDirectory();
>
> if ( !partitionsDir.exists() )
> {
> LOG.info( "partition directory doesn't exist, creating {}", partitionsDir.getAbsolutePath() );
> partitionsDir.mkdirs();
> }
>
> LOG.info( "using partition dir {}", partitionsDir.getAbsolutePath() );
>
> initSchemaLdifPartition( instanceLayout );
> initConfigPartition( instanceLayout );
>
> // Read the configuration
> cpReader = new ConfigPartitionReader( configPartition, partitionsDir );
>
> ConfigBean configBean = cpReader.readConfig( "ou=config" );
>
> DirectoryServiceBean directoryServiceBean = configBean.getDirectoryServiceBean( "default" );
>
> // Initialize the DirectoryService now
> DirectoryService directoryService = initDirectoryService( instanceLayout, directoryServiceBean );
>
> // start the LDAP server
> startLdap( directoryServiceBean.getLdapServerBean(), directoryService );
>
> // 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(), directoryService );
>
> // start the jetty http server
> startHttpServer( directoryServiceBean.getHttpServerBean(), directoryService );
> }
>
>
> Diffs
> -----
>
>
> Diff: https://reviews.apache.org/r/14/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Emmanuel
>
>