Ciao Justin, > I took an initial look over the catalog refactoring patch and here is > what I came up with.
Thanks a lot for the review. > * CatalogDAO > > The first thing I noticed and something that actually made the patch > kind of hard to review was that method names have changed: > > Catalog.add --> CatalogDAO.save > Catalog.save --> CatalogDAO.update > Catalog.remove --> CatalogDAO.delete > > Why the changes? Oops, my error. I just took as a skeleton the DAO I made for the hibernate catalog, and the names were more or less following SQL conventions. Names can easily be reverted into the standard used in the catalog. > * InMemoryCatalogDAO > > Rename to just MemoryCatalogDAO > > I notice that this class also has no notion of proxying that is > occurring. This seems odd that this is still the responsibility of > CatalogImpl. The whole point of this excercise is to have a single > CatalogImpl and multiple daos for different "storage formats". > > Since using proxy is an implementation detail of a memory based catalog > it should be totally encapsulated by that dao implementation. Ie we will > not want to proxy objects coming from the hibernate dao. This is a good point of discussion. I kept the ModProxing into the CatalogImpl in order to get easily back the old values when notifying the updates to listeners. Another way to solve this issue would be to get the old values from the DAO (may be an expensive operation), and then making a comparison of the contents. Or, yet another option, would be not to notify listeners with new and old values, and leave to the interested listeners the job of retrieving the old values from the Catalog (I guess not all the listeners are interested in them). A caching mechanism could be implemented so that such a loading is only performed once by the first listener in the chain requesting for it. Suggestions are welcome :) > * LayerGroupInfoImpl > > In getStyles() i see some code for lazily instantiating the styles list. > This pattern is not really used throughout the catalog and configuration > beans. As an alternative I think i would rather see either: > > a) XStreamPersister ensure that the styles list is property initializes > b) implement a readResolve() method that will do the instantiation OK, I wrote in the comments (jira + comments in the code) this was a tmp workaround to be discussed. All the resolve() stuff has to be refactored a bit. If it only concerns the XStreamPersister logic, we could move the resolving logic there. > * CatalogWriter, LegacyCatalogReader > > I notice that the patch modifies the storage format to put a "data" > element directly under the "catalog" root element. What is the purpose > of this? The whole point of these classes is to maintain backwards > compatability with old 1.x style data directories. I am not sure why it > would change. Well, as noted in the comment, I had to modify the XStreamPersister to make the new split work. Now, some tests uses the CatalogWriter to write files that will be read by the XStreamPersister, so I had to change it as well. The result is that XStreamPersister and CatalogWriter can write compatible formats. In the same way, LegacyCatalogReader reads file created by the them, so I had to make it a bit lenient and also accept the new format. Of course these classes will be modified again to make them work with pluggable DAOs. I you have time, let's talk a bit about it all on IRC. Ciao, Emanuele ------------------------------------------------------------------------------ SOLARIS 10 is the OS for Data Centers - provides features such as DTrace, Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW http://p.sf.net/sfu/solaris-dev2dev _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
