james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steve Brewin <sbre...@synsys.com>
Subject Re: Notes on JAMES-1352 Commits
Date Mon, 19 Dec 2011 19:33:22 GMT
Hi Eric

My responses inserted below.

Cheers
--Steve

On 19/12/2011 18:48, Eric Charles wrote:
> Hi Steve,
>
> I finally found time to look at the code and can now better realize 
> what you did :)
>
> Just giving you some loosely feedback I noted:
>
> - What's the difference between perform() and operation() methods? I'm 
> not sure to completely get it from javadoc :) - reading 
> RetryingLdapContext gives me more info, but
> - ExceptionRetryingProxy could eventually be called 
> ExceptionRetryingDelegate (deals with newDelegate()... and a proxy 
> sounds more like implementing the same api as the proxied object)
Happy to admit the Javadocs are not great on this point :(
- operation() encapsulates the bare bones behaviour
- perform() invokes the retry handler which invokes the operation() 
according to the retry schedule until it succeeds or the schedue expires.
> - Could the api reside in lifecyle-api (or data-api?) and the 
> implementation be moved to ldap module (does it make sense?)
I don't think either are a natural fit. Should we find we use this 
approach generally useful it should probably be offered up to Commons. 
As an interim measure if people don't think james-server-util is a good 
home we could establish a james-commons project. Personally I don't see 
an immediate need.
> - All methods of Dir/LdapContext need to be reimplemented with the 
> operation()/perform(). The good side I see is that the retry will 
> always work, the downside is that it demands much code writing.
Separating concerns can sometimes involve work, but it is still good to 
do. I think I mentioned somewhere in the Javadocs that using compilation 
time aspect weaving or a DynamicProxy are alternatives to explicit 
coding. Here I generated the code so that it was highly visible for 
better understanding. In my other life we would always use compilation 
time aspects.
> - Why DEFAULT_EXCEPTION_CLASSES is 
> CommunicationException.class+ServiceUnavailableException.class, and 
> not simply NamingException.class?
Because these are the only subclasses of NamingException that represent 
transient conditions and are worth retrying. The rest represent 
permanent errors.
> - Small details: the other James classes don't use _field nomenclature 
> (just field without underscore)
That is not true! Some use no prefix but not all. Used prefixes I know 
of are  '_'  and 'field'. As far as I'm aware, we do not have a policy 
on this. Were we to vote on one I would shrug my shoulders except if it 
were to enforce no prefixes, in which case I would vote against. I'll 
save my rant against this for later :)
>
> One more question: Is this common practice to have to recreate a new 
> InitialLdapContext this way, or does it exist some kind of 
> LdapConnectionPool that would handle this for us?
If the connection fails you are dead in the water. I did experiment with 
rebinding and various other strategies. None worked. The only sure way 
is to release the failed context and start again.
>
> Thx again,
>
> Eric
>
>
> On 18/12/11 19:33, Steve Brewin wrote:
>> Hi Eric
>>
>> A brief break between other duties, so here's a partial reply...
>>
>> While looking at the code you will see that one of the interfaces
>> introduced in Util is RetrySchedule. The default implementation is
>> DoublingRetrySchedule which, as the JavaDocs explain, doubles the retry
>> rest period after each failed attempt up to a set limit.
>>
>> This algorithm usually works well in the overload scenarios you
>> describe. If it doesn't, in my experience, the next algorithm to try is
>> one that randomizes the rest period within certain limits so that
>> retries from different sources don't coincide because they are following
>> the same algorithm in lock step.
>>
>> Using an interface allows the algorithm to be changed. Placing this and
>> their implementations in Util allows other things to use them. For
>> instance, RemoteDelivery could do with some reworking should someone
>> feel up to the task :)
>>
>> Cheers
>> --Steve
>>
>> On 18/12/2011 18:02, Steve Brewin wrote:
>>> Hi Eric
>>>
>>> It's probably better that you look at the code first and ask questions
>>> later. It will be quicker this way :)
>>>
>>> Cheers
>>> --Steve
>>>
>>> On 18/12/2011 18:19, Eric Charles wrote:
>>>> Hi Steve,
>>>>
>>>> Last time I connected to LDAP from JAVA code is ten years ago.
>>>> I don't remember if we were using a connection pool or whatever...
>>>>
>>>> A timeout can occur on an overloaded LDAP server, and retrying gives
>>>> a risk to make things worst as the LDAP will keep on to be more and
>>>> more overloaded due to retrying clients.
>>>>
>>>> I will take some time in coming days to look at the Tomcat's JNDI
>>>> Realm as well as to your code (and see if you retry with incremental
>>>> period of time for example).
>>>>
>>>> Can you also further elaborate on your Q2 (the interfaces in util)?
>>>>
>>>> Thx again,
>>>>
>>>> Eric
>>>>
>>>>
>>>> On 18/12/11 12:38, Steve Brewin wrote:
>>>>> Hi Eric
>>>>>
>>>>> Even when deployed in highly resilient infrastructures with multiple
>>>>> LDAP servers there is a need to retry in the case where the server
>>>>> connected to fails. In this scenario, a single instantaneous retry is
>>>>> sufficient.
>>>>>
>>>>> In less resilient infrastructures, retry provides a greater level of
>>>>> resilience. Retrying over a defined duration offers an alternative to
>>>>> having to recycle James should the LDAP server become temporarily
>>>>> unavailable.
>>>>>
>>>>> The former is common, see Tomcat's JNDI Realm. The latter easy to
>>>>> provide once support for the former is added. So why not?
>>>>>
>>>>> Cheers
>>>>> --Steve
>>>>>
>>>>> On 18/12/2011 11:09, Eric Charles wrote:
>>>>>> Hi Steve,
>>>>>> I will take more time in the next days to comment on:
>>>>>>
>>>>>> - Why do we need some retry (I know you asked the question before

>>>>>> and
>>>>>> I didn't answer by lack of time - just curious to further discuss,
>>>>>> won't ask for any revert :).
>>>>>>
>>>>>> - I saw util maven project more for utility classes, without any
>>>>>> further structure.
>>>>>>
>>>>>> Stay tuned.
>>>>>> Thx,
>>>>>>
>>>>>> Eric
>>>>>>
>>>>>>
>>>>>> On 13/12/11 12:09, Steve Brewin wrote:
>>>>>>> Hi
>>>>>>> Yesterday I committed fixes for JAMES-1352 "Increase the
>>>>>>> robustness of
>>>>>>> org.apache.james.userldap.ReadOnlyUsersLDAPRepository". A few

>>>>>>> design
>>>>>>> questions arise from this.
>>>>>>>
>>>>>>> Aside from a little house-keeping, the only changes to the
>>>>>>> org.apache.james.userldap package are:
>>>>>>> - The introduction of a retryable LDAP context via a proxy class.
>>>>>>> - Authorization uses a reconnect() on a new instance of the 
>>>>>>> existing
>>>>>>> LdapContext, thereby propagating all other Context related 
>>>>>>> settings.
>>>>>>> - All retry functionality is implemented in
>>>>>>> org.apache.james.util.retry.* packages as this retry 
>>>>>>> functionality is
>>>>>>> generic and reusable. In fact, there are no references to other

>>>>>>> James
>>>>>>> packages here.
>>>>>>>
>>>>>>> Q1: Are there any objections to factoring the generic behaviour

>>>>>>> into
>>>>>>> org.apache.james.util. as done here? If so, we can repackage.
>>>>>>> Where else
>>>>>>> should it live?
>>>>>>>
>>>>>>> Q2: I've introduced interfaces into the maven james-server-util
>>>>>>> project.
>>>>>>> Some, but not all, other projects have been split to separate

>>>>>>> classes
>>>>>>> from interfaces. Do we need to do this here? Or should we wait
>>>>>>> until the
>>>>>>> interfaces are actually used outside of the james-server-util
>>>>>>> project?
>>>>>>>
>>>>>>> Cheers
>>>>>>> Steve
>>>>>>>
>>>>>>>
>>>>>>> - - - - - - - - - - - - - - - - - -
>>>>>>>
>>>>>>> This private and confidential e-mail has been sent to you by

>>>>>>> Synergy
>>>>>>> Systems Limited. It may not represent the views of Synergy Systems
>>>>>>> Limited.
>>>>>>>
>>>>>>> If you are not the intended recipient of this e-mail and have
>>>>>>> received
>>>>>>> it in error, please notify the sender by replying with "received
in
>>>>>>> error" as the subject and then delete it from your mailbox.
>>>>>>>
>>>>>>> ---------------------------------------------------------------------

>>>>>>>
>>>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>
>>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>> For additional commands, e-mail: server-dev-help@james.apache.org
>>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message