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
