directory-kerby mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kiran Ayyagari <kayyag...@apache.org>
Subject Re: [backend] AbstractIdentityBackend interface
Date Wed, 01 Jul 2015 07:33:16 GMT
On Wed, Jul 1, 2015 at 3:15 PM, Emmanuel L├ęcharny <elecharny@gmail.com>
wrote:

> 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 :
>
> (IdentityBackend/IdentityService)
>   o
>   |
>   +-- [[AbstractIdentityBackend]]
>            ^             ^
>            |             |
>            |             +-- [[AbstractCacheableIdentityBackend]]
>            |                                 ^
>            |                                 |
>            |                                 +-- [JsonIdentityBackend]
>            |                                 |
>            |                                 +-- [LdapIdentityBackend]
>            |                                 |
>            |                                 +-- [ZookeeperIdentityBackend]
>            |                                 |
>            |                                 +-- [InMemoryIdentityBackend]
>            |
>            +-- [MavibotIdentityBackend]
>
> This is my understanding of Kiran's proposal.

yes, you did, but a minor difference, the idea is not to have an
AbstractCacheableIdentityBackend
but instead make a cache based backend that contains a cache and accepts
another instance of
backend and delegates the calls to the wrapped backend instance when a
cache miss occurs
or when an update needs to be performed.

In the end all those backedns that actually persist data are free from
cache and server
finally instantiates a cacheable backend(which takes another persisting
backend instance).

>
> * 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."
> (http://docs.oracle.com/javase/7/docs/api/java/util/LinkedHashMap.html).
> (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 !
>
>


-- 
Kiran Ayyagari
http://keydap.com

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message