On 15/06/2009, Filip Hanik - Dev Lists <devli...@hanik.com> wrote: > 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.
Not sure how calling close() multiple times relates to the threading bug (anyway, the code already guards against multiple calls from the same thread). The "closed" field is checked in the PoolCleaner thread. AFAICT, the close() method that sets the "closed" field will be called in a different thread from the pool cleaner. > > 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. I disagree that these are all personal coding preferences. For example, Javadoc errors are errors. > 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 > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
