[
http://issues.apache.org/jira/browse/DERBY-819?page=comments#action_12366695 ]
Daniel John Debrunner commented on DERBY-819:
---------------------------------------------
review derby819_2.diff
-1 veto still in place: If these BLOCKER issues are addressed, the veto is
lifted
- SystemProcedure changes need to be removed (see below)
- NPE potential at statup needs to be addressed. (see below)
- Test code just ignores the very exceptions it is meant to be testing.
(see below)
- Why is a SQLExceptionFactory and SQLExceptionFactory 40 classes needed? With
the way this is implemented, the new method
it defines could just included in InternalDriver and the 40 specific one in
Driver40. Seems a lot of overhead for little value.
BLOCKER - We still have a window where before the driver is loaded any
InternalDriver.getSQLExceptionFactory() could return null leading to a
NullPointerException. Maybe just simply closing this by having InternalDriver
initialize the static field to a factory that returns the plain SQLException
would be sufficient.
- Copyright dates for the new files need to be 2005,2006 or just 2006 depending
on when they were created
- no class javadoc for SQLExceptionFactory40
- SQLExceptionFactory40: Don't understand why there is a method
getJDBC40Exception, why not just have the
code in getSQLException
- SQLExceptionFactory40.getJDBC40Exception - No need to have the 'if (ex ==
null) ' just make ex = new SQLException
part of the else in the above block. Then no need to initialize ex = null
at the beginning of the method. This is where Java
is great, the compiler will tell you if ex is never initialized. WIth the
code as-is I have to look to see under what conditions
ex can be null for the 'if (ex == null) ' if I want to understand it.
Thus I waste my time looking for a condition that can never exist.
- Removal of all the shortcut methods seems like the wrong approach to me,
that's been discussed on the list.
It's about 90% of this patch and could have been avoided, making the real
changes easier to spot.
- Seems to be some unrelated changes included, addition of an extra parameter
here.
- throw
Util.generateCsSQLException(SQLState.INVALID_API_PARAMETER,map,"map",
+ throw InternalDriver.getExceptionFactory()
+ .generateCsSQLException(SQLState.INVALID_API_PARAMETER,map,"map",
"java.sql.Connection.setTypeMap");
BLOCKER - Very strange changes in SystemProcedures.java, look like some global
edit script gone wrong. Makes me *very* nervous about the rest of the patch,
not having the shortcut methods removed would make the patch easier to manage
and easier for the reviewers to spot issues like this.
BLOCKER - Tests have code like this:
+ } catch (SQLIntegrityConstraintViolationException e) {
+ }
How does this test that we are setting the SQLState and message correctly in
these new exceptions? Maybe the coder accidently swapped the SQLState and
message parameters.
> Provide JDBC4 SQLException subclasses support in Embedded driver
> ----------------------------------------------------------------
>
> Key: DERBY-819
> URL: http://issues.apache.org/jira/browse/DERBY-819
> Project: Derby
> Type: Sub-task
> Components: JDBC
> Environment: all
> Reporter: Anurag Shekhar
> Assignee: Anurag Shekhar
> Priority: Minor
> Attachments: .derby-819_2.stat, derby-819-onlyforreview.diff,
> derby-819.diff, derby-819_2.diff, stat.out
>
--
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
