You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by se...@verizon.net on 2004/10/17 17:48:24 UTC
[chain] CatalogFactoryBase
Craig,
Thanks for commiting those changes. I noticed you had to make a few modifications. Sorry 'bout that - did my best. One change you made I was curious about. I was using a Hashtable to store the classloaders and you changed it to Map with a synchronized block. I was curious as to why you feel this is better? Isn't Hashtable already synchronized?
Just curious.
sean
Re: [chain] CatalogFactoryBase
Posted by Michael McGrady <mi...@michaelmcgrady.com>.
Way cool! YES!
Michael McGrady
Craig McClanahan wrote:
>On Mon, 18 Oct 2004 10:48:50 -0400, Sean Schofield
><se...@schof.com> wrote:
>
>
>>Craig McClanahan wrote:
>>
>>
>>
>>>I prefer to use them throughout, so we have the opportunity to not synchronize in cases where thread safety is not an issue -- it is here, though.
>>>
>>>
>>>
>>>
>>OK makes sense.
>>
>>
>>
>>>More subtly, though, you'll also note that I use Map instead of
>>>HashMap to declare the actual instance variable, and the return values
>>>
>>>
>>>from appropriate methods. That way, the actual implementation class
>>
>>
>>>could be specialized later without breaking method signatures. That's
>>>not as easy to do if you're throwing Hashtables around (any
>>>specialized version would have to subclass Hashtable, and not some
>>>more generic interface).
>>>
>>>
>>>
>>>
>>>
>>This makes sense in general but I don't see how it applies in this
>>specific case. Only the instance variable is a Map. There aren't any
>>public methods in CatalogFactory that return Map so how does this help
>>with subclassing? I'm just wondering ... I'm not proposing changing it
>>back.
>>
>>
>
>That is true for now, but I've seen many projects (including a couple
>of my own) get bit by adding public accessors later on --
>PropertyUtils.getMappedPropertyDescriptors() returns FastHashMap
>instead of Map and we didn't catch it quick enough :-(.
>
>
>
>>Finally, I also just noticed that you implemented getInstance() in
>>CatalogFactory and had it return an instance of CatalogBase. Did you
>>change your mind on this? Is this because it will be easier to manage
>>class loader issues in the factory?
>>
>>
>>
>
>Not just easier ... the original strategy I suggested would never
>eliminate the reference to the class loader in the clear() method, so
>the garbage collection problems I spoke of would still happen. Now,
>clear() -- which is also on CatalogFactory -- throws away its own
>reference to the class loader and any loaded catalogs.
>
>
>
>>I'm assuming that your reasoning for having CatalogFactoryBase is so
>>that we (or end user) can change the implementation of CatalogFactory
>>without affecting users code. Right now user's can't really specify
>>their own CatalogFactory. I'm fine with this, but just checking to see
>>if this was your reasoning.
>>
>>
>>
>
>Yep, that's the reasoning -- future proofing the fundamental APIs a
>little. We've had to go back and try to do this thing in other
>scenarios where static utility classes were defined but couldn't be
>replaced. Now, all we'd need to do if the need came up is add a way
>to register your own CatalogFactory implementation class in order to
>create instances, if the need ever comes up.
>
>The end result of all this, by the way, is quite sweet ... no need to
>pass around references to a Catalog or a CatalogFactory, and it works
>even if [chain] is installed in a shared directory. You'll be able to
>see this too once you update your CVS checkout to pick up the
>ConfigCatalogRule.java file that I just checked in :-).
>
>
>
>>sean
>>
>>
>
>Craig
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [chain] CatalogFactoryBase
Posted by Craig McClanahan <cr...@gmail.com>.
On Mon, 18 Oct 2004 10:48:50 -0400, Sean Schofield
<se...@schof.com> wrote:
> Craig McClanahan wrote:
>
> >I prefer to use them throughout, so we have the opportunity to not synchronize in cases where thread safety is not an issue -- it is here, though.
> >
> >
> OK makes sense.
>
> >More subtly, though, you'll also note that I use Map instead of
> >HashMap to declare the actual instance variable, and the return values
> >from appropriate methods. That way, the actual implementation class
> >could be specialized later without breaking method signatures. That's
> >not as easy to do if you're throwing Hashtables around (any
> >specialized version would have to subclass Hashtable, and not some
> >more generic interface).
> >
> >
> >
> This makes sense in general but I don't see how it applies in this
> specific case. Only the instance variable is a Map. There aren't any
> public methods in CatalogFactory that return Map so how does this help
> with subclassing? I'm just wondering ... I'm not proposing changing it
> back.
That is true for now, but I've seen many projects (including a couple
of my own) get bit by adding public accessors later on --
PropertyUtils.getMappedPropertyDescriptors() returns FastHashMap
instead of Map and we didn't catch it quick enough :-(.
>
> Finally, I also just noticed that you implemented getInstance() in
> CatalogFactory and had it return an instance of CatalogBase. Did you
> change your mind on this? Is this because it will be easier to manage
> class loader issues in the factory?
>
Not just easier ... the original strategy I suggested would never
eliminate the reference to the class loader in the clear() method, so
the garbage collection problems I spoke of would still happen. Now,
clear() -- which is also on CatalogFactory -- throws away its own
reference to the class loader and any loaded catalogs.
> I'm assuming that your reasoning for having CatalogFactoryBase is so
> that we (or end user) can change the implementation of CatalogFactory
> without affecting users code. Right now user's can't really specify
> their own CatalogFactory. I'm fine with this, but just checking to see
> if this was your reasoning.
>
Yep, that's the reasoning -- future proofing the fundamental APIs a
little. We've had to go back and try to do this thing in other
scenarios where static utility classes were defined but couldn't be
replaced. Now, all we'd need to do if the need came up is add a way
to register your own CatalogFactory implementation class in order to
create instances, if the need ever comes up.
The end result of all this, by the way, is quite sweet ... no need to
pass around references to a Catalog or a CatalogFactory, and it works
even if [chain] is installed in a shared directory. You'll be able to
see this too once you update your CVS checkout to pick up the
ConfigCatalogRule.java file that I just checked in :-).
> sean
Craig
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [chain] CatalogFactoryBase
Posted by Sean Schofield <se...@schof.com>.
Craig McClanahan wrote:
>I prefer to use them throughout, so we have the opportunity to not synchronize in cases where thread safety is not an issue -- it is here, though.
>
>
OK makes sense.
>More subtly, though, you'll also note that I use Map instead of
>HashMap to declare the actual instance variable, and the return values
>from appropriate methods. That way, the actual implementation class
>could be specialized later without breaking method signatures. That's
>not as easy to do if you're throwing Hashtables around (any
>specialized version would have to subclass Hashtable, and not some
>more generic interface).
>
>
>
This makes sense in general but I don't see how it applies in this
specific case. Only the instance variable is a Map. There aren't any
public methods in CatalogFactory that return Map so how does this help
with subclassing? I'm just wondering ... I'm not proposing changing it
back.
Finally, I also just noticed that you implemented getInstance() in
CatalogFactory and had it return an instance of CatalogBase. Did you
change your mind on this? Is this because it will be easier to manage
class loader issues in the factory?
I'm assuming that your reasoning for having CatalogFactoryBase is so
that we (or end user) can change the implementation of CatalogFactory
without affecting users code. Right now user's can't really specify
their own CatalogFactory. I'm fine with this, but just checking to see
if this was your reasoning.
>Craig
>
>
sean
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [chain] CatalogFactoryBase
Posted by Craig McClanahan <cr...@gmail.com>.
Hashtable is indeed synchronized ... but it's *always* synchronized
even when you don't need it. LogFactoryBase (which I'll bet you
glanced at :-) uses Hashtable because [logging] needed to be able to
work in a JDK 1.1 environment. That's not an issue for [chain], since
we're already usng the JDK 1.2 collections classes, so I prefer to use
them throughout, so we have the opportunity to not synchronize in
cases where thread safety is not an issue -- it is here, though.
More subtly, though, you'll also note that I use Map instead of
HashMap to declare the actual instance variable, and the return values
from appropriate methods. That way, the actual implementation class
could be specialized later without breaking method signatures. That's
not as easy to do if you're throwing Hashtables around (any
specialized version would have to subclass Hashtable, and not some
more generic interface).
Craig
On Sun, 17 Oct 2004 11:48:24 -0400, sean.schofield@verizon.net
<se...@verizon.net> wrote:
> Craig,
>
> Thanks for commiting those changes. I noticed you had to make a few modifications. Sorry 'bout that - did my best. One change you made I was curious about. I was using a Hashtable to store the classloaders and you changed it to Map with a synchronized block. I was curious as to why you feel this is better? Isn't Hashtable already synchronized?
>
> Just curious.
>
> sean
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org