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 05:23:50 GMT
Thanks for the details, Kiran. 

I thought our concerns are mainly about the pattern, not the cache stuff. Regarding the cache
design, I'm totally fine with your approach. So let's focus on the pattern. Please note in
current codes, doXXX() methods are only internal ones and are not expected to be called outside.
They're protected. They don't need to do any parameter validation and precondition checking,
and only need to focus on the real tasks. They're called by the interface methods like getIdentities(start,
limit). Backend users will call getIdentities(start, limit). As a plugin and 3rdparty provided
backend, it should check the parameters and preconditions if they're robust and decent ones.
Such check should be done in getIdentities(start, limit), so I don't think they're in the
wrong level. You mentioned the checking should be in high level, which level? Between applications
and the backend, there may be a level, like cache level, as you proposed, but the using of
an extra cache level doesn't mean the underlying backend don't need to do its basic work like
parameter and precondition checking, exception handling stuffs.  Every backend may need to
do such work, so where to share the common codes? In the abstract class. It's easy to imagine
that there could be some common codes to share between backends in implementing a method like
getIdentities(), even not now, but will be soon when we have the time to refine and enhance
the backends.

It's great to have such design discussions and wish we could come to the conclusion soon.
Thanks.

Regards,
Kai

-----Original Message-----
From: Kiran Ayyagari [mailto:kayyagari@apache.org] 
Sent: Wednesday, July 01, 2015 12:16 PM
To: kerby@directory.apache.org
Subject: Re: [backend] AbstractIdentityBackend interface

On Wed, Jul 1, 2015 at 11:27 AM, Zheng, Kai <kai.zheng@intel.com> wrote:

> >> no, once we decouple this cache in AbstractIdentityBacken we don't 
> >> need
> these doXXX() anymore
>
> You said NO, but I don't think you are clear about what I had tried to 
> explain. Let me have some illustration and hope it helps.
>

> Interface IdentityBackend {
>     List<String> getIdentities(int start, int limit); }
>
> Class abstract AbstractIdentityBackend implements IdentityBackend {
>
>     public List<String> getIdentities(int start, int limit) throws 
> KrbException {
>

your example picks a valid case but implemented at a wrong level, If there is no check on
this 'start' position at the initial interceptor* level and instead was passed deep down to
the backend level then we end up with a exception thrown from bottom of the module stack rather
than preventing this call from the above said interceptor.

(* think of this interceptor as the caller who calls the method on the underlying backend)

Let me give you an example of what I am proposing

All backends will implement the IdentityBackend interface, for example the LdapIdnetityBackend
will look like

class LdapIdentityBackend implements IdentityBackend {
    // no default cache
    KrbIdentity getIdentity();
    KrbIdentity updateIdentity();
}

and then we can create a backend that exclusively makes use of cache

class CachingIdentityBackend implements IdentityBackend {
  // for now I just used a map, this can be any 'cache' library
   private Map<String, KrbIdentity> idCache;

   // the wrapped backend
   private IdentityBackend wrapped;

   // this constructor takes another backend instance that actually
   // persists data
   CachingIdentityBackend( IdentityBackend wrapped ){
     this.wrapped = wrapped;
    }

    public KrbIdentity getIdentity(String principalName) {
        // check the cache
        if (idCache.containsKey(principalName)) {
            return idCache.get(principalName);
        }

        // otherwise ask the wrapped backend
        KrbIdentity identity = wrapped.getIdentity(principalName); // no more doXXX()
     }
}

The current AbstractIdentityBackend comes with a default Map and it is a potential source
of OOM error, the above said design decouples this completely and users of Kerby can mix backends
according to their caching and storage needs. i.e If one wants to store in LDAP but wants
to cache using ehcache then he can combine them like new EhCacheIdentityBackend(new LdapIdentityBackend()));


>       // check and validate parameters start and limit, adjust them if 
> necessary
>
     // check any preconditions
>
as mentioned above these checks should not be done here but at a higher level

>     try {
>         return doGetIdentities(adjustedStart, adjustedLimit);
>     } catch (SomeException e) {
>        // convert e and throw a KrbException
>     }
>    }
> }
>
> Here, I thought doGetIdentities() works nicely and I don't think we 
> have to remove it. All the subclasses extending 
> AbstractIdentityBackend can share above block of codes and don’t have 
> to repeat the same parameter checking, exception handling and etc.
>

> The above pattern is a common OO pattern, and is widely used in Kerby 
> project. If you plan to get rid of this pattern, are you going to get 
> rid of all of them? What's the pure benefit for such change?
>
this might be a valid pattern, but we are clearly not benefiting from it here

>
> If you don't like this pattern, I would suggest you start with the 
> Mavibot backend from new, instead of extending AbstractIdentityBackend 
> that's already used by memory, json, zookeeper and ldap backends.
>
> it is not about personal preferences but bringing a consensus towards 
> a
good design,
I hope the above mentioned reasons explain my rationale behind the proposal.

> Regards,
> Kai
>
> -----Original Message-----
> From: Kiran Ayyagari [mailto:kayyagari@apache.org]
> Sent: Wednesday, July 01, 2015 10:58 AM
> To: kerby@directory.apache.org
> Subject: Re: [backend] AbstractIdentityBackend interface
>
> On Tue, Jun 30, 2015 at 1:37 PM, Zheng, Kai <kai.zheng@intel.com> wrote:
>
> > Thanks Kiran for the clarifying.
> >
> > >> no, this is a different one that stores all data in a single file 
> > >> in
> > binary format
> > >> yes, LDAP backend needs a server, whereas mavibot backend stores
> > locally on disk
> > This sounds good. Which one would ApacheDS prefer to use? This one 
> > or the LdapIdentityBackend?
> >
> when integrated with ApacheDS it initializes Kerby with 
> LdapIdentityBackend
>
> >
> > >> ic, I see that these doXXX() are for subclasses, but we can 
> > >> completely
> > avoid these methods, see below
> > I mentioned cache stuff just as a sample. Even if we get rid of the 
> > cache facility as you suggested, I thought doXXX() methods may still 
> > validate. I meant, for subclasses, they don't have to start with 
> > interface
>
> no, once we decouple this cache in AbstractIdentityBacken we don't 
> need these doXXX() anymore
>
> > methods directly every time, instead internal methods like doXXX(), 
> > because doing the way would allow sharing template codes in the 
> > abstract class.  For now, the shared codes are just cache related, 
> > later we can enhance and add more, like parameter checking, 
> > exception handling. Please note this pattern isn't rare, and is 
> > widely used across the project. That's why I still have some 
> > concerns for the
> proposed change (removing them).
> >
> there will not be any side effects with this change
>
> >
> > >> This AbstractIdentityBackend ia always utilizing a cache and this 
> > >> is
> > enforced on all backends that subclass this, instead what we should 
> > do is to create a CacheableIdentityBackend which wraps an instance 
> > IdentityBackend and maintains an internal cache, and calls are 
> > delegated to the wrapped backend when a cache miss happens.
> > >> This way implementations of IdentityBackend will be free from the 
> > >> side
> > effects of caching while testing and also become simple and cleaner.
> > This sounds great!! I'm fine to change and support the cache in this 
> > new way. I thought it's another issue to process.
> >
> > this is just part of the design, I will refactor them after 
> > finishing the
> Mavibot backend
>
> > How do you think? Thanks.
>
>
> > Regards,
> > Kai
> >
> > -----Original Message-----
> > From: Kiran Ayyagari [mailto:kayyagari@apache.org]
> > Sent: Tuesday, June 30, 2015 11:49 AM
> > To: kerby@directory.apache.org
> > Subject: Re: [backend] AbstractIdentityBackend interface
> >
> > On Mon, Jun 29, 2015 at 9:10 PM, Zheng, Kai <kai.zheng@intel.com> wrote:
> >
> > > Thanks Kiran for the taking.
> > >
> > > >> I am implementing a Mavibot based backend*.
> > > Would you clarify a bit about this? I'm wondering if it's the same 
> > > thing, the on-going LdapIdentityBackend Yaning is working on?
> > >
> > no, this is a different one that stores all data in a single file in 
> > binary format
> >
> > > Or you mean something that uses Mavibot directly instead of by the 
> > > LDAP connection API?
> > >
> > yes, LDAP backend needs a server, whereas mavibot backend stores 
> > locally on disk
> >
> > >
> > > >> Is there any reason why the API methods start with doXXX()?
> > > I would think AbstractIdentityBackend isn't the interface, and the
> > > doXXX() methods are not the APIs.
> > > Please see the APIs in the interface IdentityBackend/IdentityService.
> > >
> > ic, I see that these doXXX() are for subclasses, but we can 
> > completely avoid these methods, see below
> >
> > > AbstractIdentityBackend is a common abstract class to implement 
> > > the API interface, and provides some useful functionalities like cache.
> > >
> >
> > This AbstractIdentityBackend ia always utilizing a cache and this is 
> > enforced on all backends that subclass this, instead what we should 
> > do is to create a CacheableIdentityBackend which wraps an instance 
> > IdentityBackend and maintains an internal cache, and calls are 
> > delegated to the wrapped backend when a cache miss happens.
> >
> > This way implementations of IdentityBackend will be free from the 
> > side effects of caching while testing and also become simple and cleaner.
> >
> >
> > I thought if you don't like it, you could start with totally new,
> > > implementing IdentityBackend/IdentityService directly.
> > >
> > > I hope the above reasons make the intention behind this proposal 
> > > clear
> >
> > > >> now is the time after today's commits in Mavibot trunk that 
> > > >> address
> > > these bugs.
> > > Glad it's the time now. It will help a lot.
> > >
> > > Regards,
> > > Kai
> > >
> > > -----Original Message-----
> > > From: Kiran Ayyagari [mailto:kayyagari@apache.org]
> > > Sent: Monday, June 29, 2015 5:43 PM
> > > To: kerby@directory.apache.org
> > > Subject: [backend] AbstractIdentityBackend interface
> > >
> > > I am implementing a Mavibot based backend*.
> > >
> > > Is there any reason why the API methods start with doXXX()?
> > > This looks a bit odd and hard to read.
> > >
> > > I would like to strip the 'do' verb from these methods, please let 
> > > me know if there are any objections.
> > >
> > > * this is on hold for a long time due to the free page management 
> > > bugs, but now
> > >    is the time after today's commits in Mavibot trunk that address 
> > > these bugs.
> > >
> > > --
> > > Kiran Ayyagari
> > > http://keydap.com
> > >
> >
> >
> >
> > --
> > Kiran Ayyagari
> > http://keydap.com
> >
>
>
>
> --
> Kiran Ayyagari
> http://keydap.com
>



--
Kiran Ayyagari
http://keydap.com
Mime
View raw message