[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: Some code suggestions...



On 05/30/2013 05:04 AM, Emmanuel LÃcharny wrote:
Le 5/30/13 4:19 AM, Shawn McKinney a Ãcrit :
On 05/25/2013 04:24 PM, Emmanuel Lecharny wrote:
Hi Shawn,

I will use this mailing list for comments, suggestions and questions
regarding the code base, so that it will benefit to everyone interested.

I have a couple of suggestions I already mentionned in a chat, but it's
better to have them recorded here :

Hello Emmanuel.  Welcome to the fortress mailing list.

On 05/25/2013 04:24 PM, Emmanuel Lecharny wrote:
1) You may want ot use the LDAP connection pool to replace the pool you
are using. Mots of the LDAP API has an implementation of a pool, even
JNDI (but you don't want to suffer using it , do you ?)

Willing to consider.  Where can I find more information on the pool
you recommend?
Depends on the API you are using.

- JNDI :
http://docs.oracle.com/javase/jndi/tutorial/ldap/connect/config.html.
Ok, don't get me started :)

- UnboundId :
https://www.unboundid.com/products/ldapsdk/docs/javadoc/com/unboundid/ldap/sdk/LDAPConnectionPool.html
Although using this API is a bit problematic, as its licence is not
exactly compatible with the Fortress licence : it offers three different
kind of licences - this is the WTF of the day...- GPL (and you don *not*
want to use this -, LGPL (same kind of problem, but to a lesser extend)
and a stupid UnboundID license (" TERMINATION. UnboundID reserves the
right to discontinue offering the SDK and to modify the SDK at any time
in its sole discretion. Notwithstanding anything contained in this
Agreement to the contrary, UnboundID may also, in its sole discretion,
terminate or suspend access to the SDK to You or any end user at any
time." In other words, get lost).

- OpenDJ LDAP SDK :
http://opendj.forgerock.org/opendj-ldap-sdk/apidocs/org/forgerock/opendj/ldap/ConnectionPool.html.
A decent alternative. I mean it. CDDL, so fully compatible with ASL 2.0,
as soon as you don't do too much with it (see
http://www.apache.org/legal/3party.html)

- Apache LDAP API :
http://directory.apache.org/api/gen-docs/latest/apidocs/org/apache/directory/ldap/client/api/LdapConnectionPool.html.
Ok, hee, I'm biased :-)

- Netscape API :
http://docs.oracle.com/cd/E19957-01/816-5618-10/netscape/ldap/util/ConnectionPool.html


OK, we will put this item on our roadmap for now. Would like to use the Apache LDAP API once its ready.


On 05/30/2013 05:04 AM, Emmanuel LÃcharny wrote:
On 05/25/2013 04:24 PM, Emmanuel Lecharny wrote:
2) It would be better to get the connection immediately before using it,
and to release it as soon as you are done with it. For instance, the
connection could be get just before calling the dataProvider and
released just after. Not really a huge improvment, but assuming that
you may have thousands of requests per second, this may become an
issue.

The current pattern followed in the Fortress DAO's is as follows:

// inside any DAO method:
try
{
     1. getAdminConnection();
     2. build up the ldap attribute set or search filter
     3. perform the ldap operation
}
catch (LDAPException e)
{
    // perform error handling
}
finally
{
    4. closeAdminConnection(ld);
}

This change you are suggesting is to delay opening the connection
until the beginning of step 3?
Yes. Basically :

try
{
     1. build up the ldap attribute set or search filter
     2. getAdminConnection();
     3. perform the ldap operation

Now, it's questionable if the connection should be grab in (2) instead
in the method that performs the operation. The only rationnal I see is
for a non-admin connection to be used, but this is not the case in teh
DAO, AFAICS.

For now I have modified the code to open on #2. I went this way because this change was easy to code. I agree it would be better to wait until inside the actual operation (#3) but that will take a change to DAO base LDAP utils so will hold off for now. Perhaps we can wait until we adopt Apache LDAP SDK and kill two birds with one stone.

--
shawn.mckinney@jts.us is my new email address