[
https://issues.apache.org/jira/browse/DERBY-3324?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12561721#action_12561721
]
Knut Anders Hatlen commented on DERBY-3324:
-------------------------------------------
The code looks correct to me, and the unit tests seem to cover the code well.
Some comments/suggestions follow below (mostly stylistic, feel free to ignore).
If the cache always creates physical CallableStatement objects internally
(except for prepared statements with auto-generated keys, which aren't
supported by CallableStatement), and creates logical prepared/call wrappers
around them, it doesn't need different keys for prepared and call statements,
so the class hierarchy could be simplified (no need for CallKey). Just a
thought...
I'm not sure I see the value of such a complex class hierarchy for the
statement keys. First there is a StatementKey interface which is essentially
empty (only contains methods already in java.lang.Object). Then there's an
AbstractStatementKey class which contains some basic information about the
statement. And then finally three different sub-classes BasicKey (for prepared
statements without generated keys), AutoGeneratedKeysKey (for prepared
statements with generated keys) and CallKey (for callable statements), all of
which only override equals() and hashCode() so that they consider the sub-type.
I feel that the separation into different classes makes it harder to read the
code. Also, it feels a bit arbitrary whether a property is represented in a
field or as a sub-class. Wouldn't it be simpler just to add two fields to
AbstractStatementKey
- callable (which could also be skipped, see above)
- autoGeneratedKeys
and merge the five classes/interfaces into a single class StatementKey?
Other comments:
BasicKey and AutoGeneratedKeysKey have identical hashCode() methods (31 *
super.hashCode()). Although it's not a bug, it's probably better to use
different numbers.
AbstractStatementKey and subclasses:
+ * @throws IllegalArgumentException if <code>sql</code> and/or
+ * <code>schema</code> is <code>null</code>
I think it is more common (at least in the standard Java API) to throw
NullPointerException instead of IllegalArgumentException when an argument is
(illegally) null.
JDBCStatementCache.cacheStatement:
+ * @param ps prepared statment to cache
typo: statment -> statement
> JDBC statement cache implementation
> -----------------------------------
>
> Key: DERBY-3324
> URL: https://issues.apache.org/jira/browse/DERBY-3324
> Project: Derby
> Issue Type: Sub-task
> Components: Network Client
> Affects Versions: 10.4.0.0
> Reporter: Kristian Waagan
> Assignee: Kristian Waagan
> Priority: Minor
> Fix For: 10.4.0.0
>
> Attachments: derby-3324-1a-jdbc_statementcache.diff,
> derby-3324-1a-jdbc_statementcache.stat,
> derby-3324-1b-jdbc_statementcache.diff, derby-3324-1b-jdbc_statementcache.stat
>
>
> Implement a cache for storing JDBC prepared statement objects.
> The cache will be responsible for holding free prepared statement objects
> that can be reused, and also to throw away objects if the cache grows too big.
> All objects in the cache must belong to the same physical connection, but
> they can be reused across logical connections obtained from a single physical
> connection in a connection pool.
> This component is probably a candidate for code sharing between the client
> and the embedded driver. Sharing will not be part of this issue.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
