directory-kerby mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel L├ęcharny <>
Subject Re: [backend] AbstractIdentityBackend interface
Date Wed, 01 Jul 2015 07:15:09 GMT
Hi Kiran, Kai,

just reading this thread (for some weird reason, my mail client wasn't
refreshing this mailing list, so I have missed it from day one...)

I have reviewed the code and I'd like to add my perception.

* the doXXX method

I don't see anything wrong on having protected doXXX methods in such a
code, where you have some Interfaces describing the contract, some
implementation classes and an abstract layer that gather some common
beahviour and delegates what is specific to the implementation classes
through doXXX methods.

As Kai says, this is a very common pattern.

I think that Kiran expressed his concern - which is real, but bear with
me, I'll come to that later - the wrong way, by pointing to those doXXX
in his first mail. IMHO, the name is not a problem, and I interpret
Kiran's first mail this way : "the doXXX methods, as part of an API, are
less readable than XXX". Kai, you correctly explained that those doXXX
methods are not part of the API

* the Cache

Now, let's get to the real pb : what Kiran says, and I think he is
correct, is that the only reason those doXXX methods exists is for the
abstract class to implement a cache, which will b updated after the
impelmentation doXXX method has been called.

Let's do some mental exercise now : let say we don't have a cache at
all. In this case, we immediatly see that those doXXX methods are
useless. We can simply replace the doXXX methods in the implementations
by the XXX methods, which is part of the API, instead of having to go
through the abstract class.

So far, so good. But what if we need a cache, then ? That's a very valid
concern. I would expect that, for performances reason, we don't pound
the backend every time we need to get an identity. The cache serves this
purpose, AFAICT. So let's say xe need a cache.

At this point, this is the real question Kiran is raising : if we need a
cache, where should this cache been implemented ?

* When and where should we implement a cache ?

That's an important point. There are two reasons for having a cache :
because we want to answer fast to a client request, and because the
backend does not have such a cache.

Now, that's one of Kiran's point : as he is to implement a Mavibot
implementation, he does not need a cache, as Mavibot already implement
this cache. His suggestion then is quite natural : we should have two
categories of abstract classes : one with a cache, one with no cache,
and the inheritance schema should be like :

  +-- [[AbstractIdentityBackend]]
           ^             ^
           |             |
           |             +-- [[AbstractCacheableIdentityBackend]]
           |                                 ^
           |                                 |
           |                                 +-- [JsonIdentityBackend]
           |                                 |
           |                                 +-- [LdapIdentityBackend]
           |                                 |
           |                                 +-- [ZookeeperIdentityBackend]
           |                                 |
           |                                 +-- [InMemoryIdentityBackend]
           +-- [MavibotIdentityBackend]

This is my understanding of Kiran's proposal.

* the cache (again) :-)

Ok, let's go back to teh cache current implementation : it's *BAD*.
Seriously. Kerby is meant to be used in a multi-threaded environement,
and AFAICT, I see no protection against concurrent access to the cache,
something which *will* happen. That means you will face complex issues
when the cache is read and updated at the same time. When you look at
the code, you see that the cache is using a LinkedHashMap, which is
explicitely said to be not thread safe : " *Note that this
implementation is not synchronized.* If multiple threads access a linked
hash map concurrently, and at least one of the threads modifies the map
structurally, it /must/ be synchronized externally."
(I urge you guys to always think about such issue when you write code...)

Now, even a cache can be multiform : it would be very helpful to
abstract the cache implementation so that it can be swapped later. That
would be easier with this cache proxy class
(AbstractCacheableIdentityBackend), which could be configured with
whatever cache we want (be it a LRUMap, ehcache, or whatever we want).

This is not easy, some discussion about what would be the best approach
would be great...

* The doXXX methods (again ;-)

Ok, now that we have seen what is the crux of the pb (the cache), we can
come back to what is at the periphery. Here, what Kiran says is that if
we don't put the cache in the top abstract class, then the doXXX methods
in this abstract class are useless. Now, if we have this
AbstractCacheableIdentityBackend class, which handle requests through a
cache, then those doXXX method are making sense for the implementation
classes extendending the abstract class.

Bottom line : if you are not going to need it, don't do it.

* About the code...

Ok, as I have looked at the code (sorry, being quite busy at this
moment, I haven't done that for a while, my bad), and I have to stress
out that the Javadoc *is* an important part of it. And here, there is
some effort to do :

- fields are not documented
- most of the methods are not documented

I do understand that the Interface are documented, but for someone
having a look at the code, it's hard to tell if one specific method is
implementing a documented Interface. What we usually do is to add such a
comment :

     * {@inheritDoc}

that allows you to avoid adding some documentation that is already
present in the interface, and it gives a clear clue to a reader that
this method is documented in an interface (or abstract class).

That's it, enough for a mail ! I hope I have captured the spirit of what
Kiran wanted to express, and I hope that I made it clear for both of
you. Bottom line, there is not one single way of doing things, and even
more : there can be many ways that are all valid...

Thanks !

View raw message