db-jdo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Adams" <matthew.ad...@xcalia.com>
Subject RE: Review of Matthew's patch
Date Mon, 02 Apr 2007 16:42:35 GMT
Inline... 

>-----Original Message-----
>From: Craig.Russell@Sun.COM [mailto:Craig.Russell@Sun.COM] 
>Sent: Friday, March 30, 2007 2:09 PM
>To: JDO Expert Group; Apache JDO project
>Subject: Review of Matthew's patch
>
>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.
>
Let's discuss.

>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  
>persistence.xml.
>
Same response.  Let's discuss.

>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  
>(matthew.adams@home).
>
Fixed in patch after my first commit.

>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.
>
Fixed in patch after my first commit.

>5. +                catch (IOException ioe) { /* gulp */ }
>I don't remember talking about this. Shouldn't we wrap this in a  
>JDOSomethingException?
>
This is only there when attempting to close the input stream in the
finally block.  It's modeled after other code I saw that closed input
streams in finally blocks.  I'll leave it unless you want to try to
throw exceptions in finally blocks that may be executed after an
exception is already being handled.

>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.
>
This would be a nice enhancement.  I'll look into it.  Did you mean
jdoconfig.xml instead of persistence.xml?

Mime
View raw message