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:[email protected]] Sent: Wednesday, July 01, 2015 3:15 PM To: [email protected] 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 !
