Sounds good to go. Would we have a branch for this? Thanks. Regards, Kai
-----Original Message----- From: Kiran Ayyagari [mailto:[email protected]] Sent: Wednesday, July 01, 2015 3:33 PM To: [email protected] Subject: Re: [backend] AbstractIdentityBackend interface On Wed, Jul 1, 2015 at 3:15 PM, Emmanuel Lécharny <[email protected]> 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
