You are viewing a plain text version of this content. The canonical link for it is here.
Posted to kerby@directory.apache.org by Kiran Ayyagari <ka...@apache.org> on 2015/06/29 11:43:17 UTC
[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
Re: [backend] AbstractIdentityBackend interface
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/07/15 09:33, Kiran Ayyagari a écrit :
> On Wed, Jul 1, 2015 at 3:15 PM, Emmanuel Lécharny <el...@gmail.com>
> 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).
>
That's definitively an option.
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
I'm going to address some left work discussed here done in this issue:
https://issues.apache.org/jira/browse/DIRKRB-347
Regards,
Kai
-----Original Message-----
From: Kiran Ayyagari [mailto:kayyagari@apache.org]
Sent: Wednesday, July 01, 2015 5:25 PM
To: kerby@directory.apache.org
Subject: Re: [backend] AbstractIdentityBackend interface
On Wed, Jul 1, 2015 at 5:17 PM, Emmanuel Lécharny <el...@gmail.com>
wrote:
> Le 01/07/15 10:49, Kiran Ayyagari a écrit :
> > On Wed, Jul 1, 2015 at 4:41 PM, Zheng, Kai <ka...@intel.com> 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)
>
> I'll suggest a branch, or better, a pull request. This is so easy in
> git that it would be lame not to use this !
>
ah yes, GIT :)
--
Kiran Ayyagari
http://keydap.com
Re: [backend] AbstractIdentityBackend interface
Posted by Kiran Ayyagari <ka...@apache.org>.
On Wed, Jul 1, 2015 at 5:17 PM, Emmanuel Lécharny <el...@gmail.com>
wrote:
> Le 01/07/15 10:49, Kiran Ayyagari a écrit :
> > On Wed, Jul 1, 2015 at 4:41 PM, Zheng, Kai <ka...@intel.com> 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)
>
> I'll suggest a branch, or better, a pull request. This is so easy in git
> that it would be lame not to use this !
>
ah yes, GIT :)
--
Kiran Ayyagari
http://keydap.com
Re: [backend] AbstractIdentityBackend interface
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/07/15 10:49, Kiran Ayyagari a écrit :
> On Wed, Jul 1, 2015 at 4:41 PM, Zheng, Kai <ka...@intel.com> 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)
I'll suggest a branch, or better, a pull request. This is so easy in git
that it would be lame not to use this !
Re: [backend] AbstractIdentityBackend interface
Posted by Kiran Ayyagari <ka...@apache.org>.
On Wed, Jul 1, 2015 at 4:41 PM, Zheng, Kai <ka...@intel.com> 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:kayyagari@apache.org]
> Sent: Wednesday, July 01, 2015 3:33 PM
> To: kerby@directory.apache.org
> Subject: Re: [backend] AbstractIdentityBackend interface
>
> On Wed, Jul 1, 2015 at 3:15 PM, Emmanuel Lécharny <el...@gmail.com>
> 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
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
Sounds good to go. Would we have a branch for this? Thanks.
Regards,
Kai
-----Original Message-----
From: Kiran Ayyagari [mailto:kayyagari@apache.org]
Sent: Wednesday, July 01, 2015 3:33 PM
To: kerby@directory.apache.org
Subject: Re: [backend] AbstractIdentityBackend interface
On Wed, Jul 1, 2015 at 3:15 PM, Emmanuel Lécharny <el...@gmail.com>
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
Re: [backend] AbstractIdentityBackend interface
Posted by Kiran Ayyagari <ka...@apache.org>.
On Wed, Jul 1, 2015 at 3:15 PM, Emmanuel Lécharny <el...@gmail.com>
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
Re: [backend] AbstractIdentityBackend interface
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/07/15 10:31, Zheng, Kai a écrit :
>>> you should protect the cache no matter what, even the query part. What would happen if you are reading the cache while another thread is reading it ?
> It's the same meaning with what I meant when said, "the LinkedHashMap should be protected since there may be many querying that need to update the cache".
>
> In addition to that, I mentioned the two cases of using the backend APIs to explain why those interface methods are not synchronized. I would add some comments about this later.
Probbaly some misunderstanding on my side...
What I wanted to say is that any method which manipulate the cache
(either for read or wraite) must be thread safe. I guess this is what
you have in mind to.
Also note that Synchronize is not necessarilly the best tool for that.
Locks could be good enough, and we can even thing about using a
ThreadLocalStorage to store the cache, so that we don't need any
synchronization at all (but the price is that you will eat more memory).
Thanks Kai !
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
>> you should protect the cache no matter what, even the query part. What would happen if you are reading the cache while another thread is reading it ?
It's the same meaning with what I meant when said, "the LinkedHashMap should be protected since there may be many querying that need to update the cache".
In addition to that, I mentioned the two cases of using the backend APIs to explain why those interface methods are not synchronized. I would add some comments about this later.
>> The best is to be sure any of the committers take care of it *before* committing. I know it's a tedious task, but less than doing so afterward...
I agree. We should add missing docs (not just docs, but also logs!!) for existing codes. We're working on that for the release. For new patches, we should guard if necessary Java docs are there.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Wednesday, July 01, 2015 4:04 PM
To: kerby@directory.apache.org
Subject: Re: [backend] AbstractIdentityBackend interface
Le 01/07/15 09:58, Zheng, Kai a écrit :
> 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.
you should protect the cache no matter what, even the query part. What would happen if you are reading the cache while another thread is reading it ?
>
> 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.
The best is to be sure any of the committers take care of it *before* committing. I know it's a tedious task, but less than doing so afterward...
Re: [backend] AbstractIdentityBackend interface
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/07/15 09:58, Zheng, Kai a écrit :
> 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.
you should protect the cache no matter what, even the query part. What
would happen if you are reading the cache while another thread is
reading it ?
>
> 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.
The best is to be sure any of the committers take care of it *before*
committing. I know it's a tedious task, but less than doing so afterward...
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
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:elecharny@gmail.com]
Sent: Wednesday, July 01, 2015 3:15 PM
To: kerby@directory.apache.org
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 !
Re: [backend] AbstractIdentityBackend interface
Posted by Emmanuel Lécharny <el...@gmail.com>.
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 !
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
How about making those doXXX() methods as empty and not abstract, so subclasses have the freedom of skipping them, overriding and implementing the interface methods directly? When have the new codes in hand, we may be more clear which way is better and how to consolidate them then.
-----Original Message-----
From: Zheng, Kai [mailto:kai.zheng@intel.com]
Sent: Wednesday, July 01, 2015 1:24 PM
To: kerby@directory.apache.org
Subject: RE: [backend] AbstractIdentityBackend interface
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 <ka...@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 <ka...@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 <ka...@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
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
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 <ka...@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 <ka...@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 <ka...@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
Re: [backend] AbstractIdentityBackend interface
Posted by Kiran Ayyagari <ka...@apache.org>.
On Wed, Jul 1, 2015 at 11:27 AM, Zheng, Kai <ka...@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 <ka...@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 <ka...@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
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
>> 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 {
// check and validate parameters start and limit, adjust them if necessary
// check any preconditions
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?
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.
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 <ka...@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 <ka...@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
Re: [backend] AbstractIdentityBackend interface
Posted by Kiran Ayyagari <ka...@apache.org>.
On Tue, Jun 30, 2015 at 1:37 PM, Zheng, Kai <ka...@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 <ka...@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
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
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?
>> 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 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).
>> 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.
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 <ka...@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
Re: [backend] AbstractIdentityBackend interface
Posted by Kiran Ayyagari <ka...@apache.org>.
On Mon, Jun 29, 2015 at 9:10 PM, Zheng, Kai <ka...@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
RE: [backend] AbstractIdentityBackend interface
Posted by "Zheng, Kai" <ka...@intel.com>.
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?
Or you mean something that uses Mavibot directly instead of by the LDAP connection API?
>> 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.
AbstractIdentityBackend is a common abstract class to implement the API interface, and provides some useful functionalities like cache.
I thought if you don't like it, you could start with totally new, implementing IdentityBackend/IdentityService directly.
>> 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