[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12314354 ]
David Van Couvering commented on DERBY-336: ------------------------------------------- Hi, Dyre. I'm kicking off a build and will let you know. In the meantime, here are my comments on the changes. For the most part this looks good, it's nice to see these cleaned up. A couple of comments: --- In RAFContainer.java, you modified one message to say "(see nested exception)". It seems to me the last argument of FILE_CANNOT_REMOVE_FILE should not be there if all we're going to say is "see nested exception". We should either remove this argument, or provide the message string. Note that the writer of this message may have intended for the exception message to be printed out in the exception string and that the user could find the details of the exception (e.g. the stack trace) by looking in the error log. My personal preference would be to include the exception message, better to provide more detail rather than less when it comes to errors. -- The FILE_CREATE_NO_CLEANUP is a strange beast. It is only used in *one* single place in the code with both exceptions and is creating some real complexity in the StandardException.newException() methods. It seems to me that if all you want to show is the message of an exception, you should pass the exception string as an argument, e.g. StandardException.newException(SQLState.FILE_CREATE_NO_CLEANUP, ioe, file, se.getMessage()); If you modified the once instance where two exceptions are passed to use this approach, then you wouldn't need this specialized version of newException() two exception objects. You could just use the one that takes (messageID, Throwable, Object, Object). --- I understand what you're trying to accomplish with the "dummy" overloads, but it makes me a bit nervous. It's an odd use of the checked exception model. That said, I think it does work to catch improper usage of the StandardException.newException(). I just wish there was another way to do this... One approach would be to have a setContainedException() method on StandardException and have a policy that newException *only* takes arguments to the message string. I think this is a better model than the "fake" constructor methods, but the drawback is that it would likely require a significant rewrite of existing code that pass exceptions into StandardException. Thanks, David P.S. I may have other comments on your comment above, but I wanted to focus on your initial patch first. > The wrong overload of StandardException::newException() is used in some cases > ----------------------------------------------------------------------------- > > Key: DERBY-336 > URL: http://issues.apache.org/jira/browse/DERBY-336 > Project: Derby > Type: Bug > Environment: Any > Reporter: Dyre Tjeldvoll > Assignee: Dyre Tjeldvoll > Priority: Trivial > Attachments: derby-336.diff, derby-336.stat, derbyall_report.txt > > When looking at DERBY-128 it became clear that the wrong overload of > StandardException::newException() was used when reporting > SQLState.SERVICE_DIRECTORY_CREATE_ERROR. The message string only takes one > parameter so only one additional parameter (other than Throwable) should be > used: > PersistentServiceImpl.java:676 > throw > StandardException.newException(SQLState.SERVICE_DIRECTORY_CREATE_ERROR, > > serviceDirectory, null); > // Calls StandardException.newException(String, Object, Object) > // Should call StandardException.newException(String, Object)? Or > StandardException.newException(String, Throwable, Object)? With the > IOException as > // Throwable? > PersistentServiceImpl.java:692 > throw > StandardException.newException(SQLState.SERVICE_DIRECTORY_CREATE_ERROR, name, > t); > // Calls StandardException.newException(String, Object, Object) > // Should call StandardException.newException(String, Throwable, Object)? > BaseDataFileFactory.java:279 > throw StandardException.newException( > SQLState.SERVICE_DIRECTORY_CREATE_ERROR, dataDirectory, ioe); > // Calls StandardException.newException(String, Object, Object) > // Should call StandardException.newException(String, Throwable, Object)? -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
