Sebb, thanks for the feedback.
sebb wrote:
Apart from the problems with N&L files etc that have already been
mentioned, I found the following:
==
The changelog.html file refers to Tomcat JDBC Connection Pool
*v1.0.5-beta* which looks wrong.
fixed.
==
The documentation for the Interceptors appears to be incorrect.
For example:
The code sample:
jdbcInterceptors="ConnectionState;StatementFinalizer(useWeakReferences=true,useEquals=true)"
implies that StatementFinalizer has a property useWeakReferences, but
that does not appear to be the case - and it is not documented in any
sections below.
fixed.
Also SlowQueryReport does not have a threshold attribute.
sure does. it inherits it.
There may be other inconsistencies; I did not check it all
The Copyright years in the html files are wrong.
fixed
==
As to the source:
There does not appear to be a build file or any test cases so it's not
easy to check if the code works as intended. I was able to build the
POJO sample, and it worked against Apache Derby after making the
necessary changes. However this does not constitute a thorough test.
Added in docs and references to the build file.
There are some threading issues, e.g. the ConnectionPool.closed
boolean field is accessed by multiple threads, but is not synchronized
or volatile. This will probably work most of the time, but is not
guaranteed to work.
yes, although calling close multiple times would be a programmer error.
It's confusing to have two classes called ConnectionPool (or is this
required by JMX/MBean?)
There is a naming convention between the MBean interface and the MBean
itself.
Doesn't have to be ConnectionPool, it just ended up being that way.
Thanks for taking a look.
The rest is personal preferences in coding. And some ol' dogs like me,
don't like to learn new tricks, only fix bugs.
Filip
Likewise, the class name org.apache.tomcat.jdbc.pool.DataSource
shadows the simple name of the implemented interface
javax.sql.DataSource. This can cause confusion.
==
There are some dubious coding practices, e.g.
The method Statement#close() throws SQLException, yet the code catches
and ignores Exception (e.g. in StatementFinalizer).
The fields:
interceptor.AbstractCreateStatementInterceptor.statements and
interceptor.AbstractCreateStatementInterceptor.executes
are both public arrays, which anyone can overwrite.
==
The code uses generics, but there are some instances of raw types being used.
Also a few unnecessary casts, and some unchecked casts.
There are a lot of Javadoc errors, and quite a few unused imports.
The visibility of fields seems to be generally too permissive; it's
much harder to test and maintain code that has lots of non-private
variables. Some of the classes have public getters and setters yet the
fields they operate on are not private.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
