directory-kerby mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zheng, Kai" <kai.zh...@intel.com>
Subject RE: [backend] AbstractIdentityBackend interface
Date Wed, 01 Jul 2015 08:41:28 GMT
Sounds good to go. Would we have a branch for this? Thanks.

Regards,
Kai

-----Original Message-----
From: Kiran Ayyagari [mailto:kayyagari@apache.org] 
Sent: Wednesday, July 01, 2015 3:33 PM
To: kerby@directory.apache.org
Subject: Re: [backend] AbstractIdentityBackend interface

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
View raw message