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-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
