Hi Justin (hey all), here is the (in)famous file by file patch review :-p General --------------
* Most classes seem to miss the GeoServer license headers. This should be addressed before committing * How are Hibernate lazy initializations and proxies managed? As far as I can see * Following up on the synchronization blocks inside the catalog: yeah, we'd definitely need transactions if we want that to work fine also in memory. Hibernate catalog wise we could use an "open session in view" approach and just have a servlet filter open the session and a transaction for us without having to roll a catalog transaction api. The current setup, with transaction marked on the dao implementations (via annotations) is not going to work fine, the synchronization in the catalog does not give any guarantee that the changes are going to occur properly on the db. For example, if the network goes down during one of those synch blocks and just one of the dao commands were sent away, well, bye bye, you'd get your database in an inconsistent state. I'd say this one is the only really serious problem I've seen in the review, and one that should be addressed with some urgency (if we go for a "open session in view" approach it should not be too hard to address too). * I see some code trying to handle a migration from file based to datbase based. Where is the migration happening? I see HibGeoServerLoader loading all of the information from the old catalog, but how is that stored back in the db? Details details -------------------- * The dbconfig pom seems to have a number of commented out dependencies. A quick cleanup seems in order. * in AbstractHibDao.first(query, doWarn) the doWarn param is not really used to control exception reporting, an exception is always thrown * In the same method there is some paranoid logging that was probably useful only to the developer during the building of the class? At least it's odd to see it only there and not, for example, in list(query) * HibPostLoadEventListener has an unused field, appContext * FilterType uses XML encoding to store the filters. I'm wondering, what happens if we have an idfilter mixed with other filters? Official encoding does not support that case, but we're good as long as we can round trip that case in xml somehow. ECQL could be another candidate for filter storage (but it has problems with field names in other locales, which I guess is not the case for xml encoding) * ClassMappings implementation looks odd to me.... why not use two fields and create a class mapping with a WORKSPACE(WorkspaceInfo.class, WorkspaceInfoImpl.class) instead of using a raft of inner classes? Would seem simpler to implement, easier to read, and use less bytecode in the end * persistence.xml has a commented out block about the mapping file * dbConfig own applicationContext.xml hard codes the references to a H2 database located in the data directory. Is this going to become configurable so that one can point GeoServer to an external database without the need to unpack the jars, modify the files, pack them again and restart? * in mappings.hbm.xml there is a ton of " <!--cache usage="read-write"/-->" What is going on there? :-) There is also a mappings.hbm.xml.orig which should not be there long term wise * the github diff contains a "community/monitoring/GeoLiteCity.dat" file that should not be there (btw, is that a geolocation database? :-) ) * The patch set contains a H2 dialect. As far as I can see here: http://opensource.atlassian.com/projects/hibernate/browse/HHH-3401 in 3.3.2 the H2 dialect was finally fixed. Maybe this class is no longer needed? Closing up ---------------- Most of the feedback above is on secondary importance details, meaning the overall setup is good ;-) The current transaction management worries me, I'm not saying it has to be improved right now but it should at least be put at some high priority in the near future, before we start advertising that we have a "rock solid" db storage for catalog and configuration Cheers Andrea ----------------------------------------------------- Ing. Andrea Aime Senior Software Engineer GeoSolutions S.A.S. Via Poggio alle Viti 1187 55054 Massarosa (LU) Italy phone: +39 0584962313 fax: +39 0584962313 http://www.geo-solutions.it http://geo-solutions.blogspot.com/ http://www.linkedin.com/in/andreaaime http://twitter.com/geowolf ----------------------------------------------------- ------------------------------------------------------------------------------ Nokia and AT&T present the 2010 Calling All Innovators-North America contest Create new apps & games for the Nokia N8 for consumers in U.S. and Canada $10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store http://p.sf.net/sfu/nokia-dev2dev _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
