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 07:58:58 GMT
Thanks Emmanuel for taking care of this and the comprehensive understanding! I thought you're
all right.

As I said before, I'm totally OK to redesign and re-implement the cache support since the
original is a quick work and too simple. I thought both yours or Kiran's are good to me. 

Yes in the existing codes, the LinkedHashMap should be protected since there may be many querying
that need to update the cache. It's my fault not seeing this. Note most of the methods don't
need synchronized because the interface contains two parts of APIs, one is for KDC which is
mainly for querying, the other is for kadmin that's to add/update/delete entries. No concurrent
threads in kadmin would be used I guess.

Yes again we need the Javadocs. Recently I'm revisiting some of the codes and refined quite
much. I'm going to revisit them again to add the missing Javadocs particularly for important
APIs. To avoid conflict with Kiran's effort I'm hesitating on the Identity backend part.

Regards,
Kai

-----Original Message-----
From: Emmanuel L├ęcharny [mailto:elecharny@gmail.com] 
Sent: Wednesday, July 01, 2015 3:15 PM
To: kerby@directory.apache.org
Subject: Re: [backend] AbstractIdentityBackend interface

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.

* 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 !

Mime
View raw message