On Wed, Jul 1, 2015 at 4:41 PM, Zheng, Kai <[email protected]> wrote:
> Sounds good to go. Would we have a branch for this? Thanks. > I don't think so, we can do this after my refactoring, (by this week's end) Just to be clear, I will not refactor until after committing the Mavibot backend. I will implement this first based on the current abstract hierarchy and commit the refactored version later. > 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 > -- Kiran Ayyagari http://keydap.com
