tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Hanik - Dev Lists <devli...@hanik.com>
Subject Re: [VOTE] Release JDBC Pool module v1.0.3
Date Mon, 15 Jun 2009 17:22:25 GMT
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-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
>   


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message