Here is my feedback too: I think we both got sidetracked by focusing just on the database case (where a really smart DataSource would take care of the caching of connections & prepared statements). Not all DirectAuthorityFactory implementations are so lucky ... We should be able to configure a BufferedAuthorityFactory with a pool of two WKTAuthorityFactory instances.
So I am strongly in favour of the ObjectPool alternative (basically the proposal as it stands). The "Fire and Forget" alternative is only applicable to the database case when we have a smart datasource. The problems basically occur at different levels in the class diagram. Martin Desruisseaux wrote: > After more though and a quick look in to code for refreshing my memory, there > is > whats look like my preference at this time: > > ReferencingObjectPool > --------------------- > Maybe we should write a generic "ObjectPool" (or "Cache") class instead and > put > in into org.geotools.util package. The current "BufferedAuthorityFactory.pool" > and "BufferedAuthorityFactory.findPool" fields would be two distinct instances > of "ObjectPool". > I see - that makes sense. We need to call this something else ... because "ObjectPool" already means something formal in computer science speak - it is a technique used by factories to recycle objects that undergo a lifecycle of some sort (a database connection must be checked out of a datasource and then closed, a wkt.property file must be opened and closed etc...) So you are correct "Cache" is the correct word - ReferencingObjectCache. A cache is used with immutable data objects for performance reasons, a factory will remember previous instances (and since the objects are immutable) will be able to return the requested instance faster if it has previously been created. > FactoryUsingSQL > --------------- > If we can rely on PreparedStatement pooling either through a JDBC 3.0 > compliant > DataSource or through a DBCP wrapper, then we could try to remove the > Connection > and PreparedStatement cache from FactoryUsingSQL. Together with the > replacement > of all other HashMap caches by the above-cited generic ObjectPool cache > (except > maybe 'safetyGuard' that we could wrap in a ThreadLocal), it may allow us to: > > * Remove most "synchronized" statements, except maybe in createObject(...) > but I guess that 90% of usage would be > createCoordinateReferenceSystem(...) > instead. > > * Remove the TimerTask used for closing connection in > DeferedAuthorityFactory > (or maybe we would keep it for other kind of factory, like the ones backed > by property files...) > > * Remove the shutdown hook in DefaultFactory. > > Relationship between FactoryUsingSQL and BufferedAuthorityFactory > ----------------------------------------------------------------- > It has been suggested to replace this relationship by FactoryUsingSQL holding > a > reference to ObjectPool. They are no technical reason for that as far as I > know; > the only purpose is that current relationship is considered confusing. > > I think you may be incorrect? Did I draw my picture wrong? Oh wait we are confusing object pool and cache ... We were talking about having the workers be able to check the cache on their own; because in fact the relationship is confusing ... in the current case with a single worker this is okay. In the case where we get multiple workers recursion opens up too many opportunities for confusion. So the option of *injecting* the cache into the worker is a good one; the cache is responsible for synchronization on write etc... Cory is writing up a sequence diagram showing this now. > Note that injecting a ObjectPool cache in FactoryUsingSQL essentially > transform > FactoryUsingSQL into a BufferedFactoryUsingSQL, which may be seen as someting > against the "separation of concern" principle - wrapping that into a > BufferedAuthorityFactory would become close to useless. But on the other hand, > FactoryUsingSQL is already using a number of other HashMap as caches. > > You are correct; having a cache makes it buffered. The thing that is missing here is that the BufferedAuthorityFactory is really acting as a mediator - it organizes the and dispatches work to the workers; it has owns the shared cache - so if the work is already done it does not have to even ask a worker etc... So the separation of concerns is still there. Going to come back to this email - I have a meeting right now. Jody ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
