db-jdo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Craig L Russell <Craig.Russ...@Sun.COM>
Subject Review of Matthew's patch
Date Fri, 30 Mar 2007 21:09:25 GMT
Nice job Matthew. Good code, nice style, we should definitely keep  
you on the team. Any more where you come from?

I think this is good to check in. We can resolve details with patches.

1. I'd like to have a name attribute for the PMF. Seems like this is  
a top level concept when finding PMF by name. This would be  
equivalent to the PUName that we've defined already. I'm not sure  
what we should do with the existing PUName. We could allow the user  
to specify the name as an attribute and check it where we check for  
PUName in attribute or property and allow only one of them.

2. The same comment applies to the provider class. We might consider  
promoting the javax.jdo.PersistenceManagerFactoryClass to "provider".  
Similarly, let's look at all the elements of persistence-unit in  

3. +        catch (ParserConfigurationException e) {
+            throw new JDOFatalUserException(
+                msg.msg("EXC_ParserConfigException"),
+                e);
+        }

Are you sure this is a user error? Could be a  
JDOFatalInternalException that should be reported to  

4. +        catch (SAXException e) {
+            throw new JDOFatalUserException(
+                msg.msg("EXC_ParsingException", url.toExternalForm()),
+                e);
The SAX parser exception message should include the line number and  
column number of the error. This should be available from the  
exception itself but for some reason is not in the message text.

5. +                catch (IOException ioe) { /* gulp */ }
I don't remember talking about this. Shouldn't we wrap this in a  

6. Not that there's anything intrinsically wrong with your  
implementation, but I'd like to have a way for the jdo implementation  
to provide a different persistence.xml handler. Like add a method to  
JDOImplHelper to registerJDOConfigHandler, and refactor the existing  
handler to statically register itself. To do this, the jdoconfig  
handler needs to have a single instance with the methods you've  
created to do the parsing.

Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:Craig.Russell@sun.com
P.S. A good JDO? O, Gasp!

View raw message