Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: pass (athena.apache.org: domain of sebbaz@gmail.com designates
 209.85.219.221 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :content-type:content-transfer-encoding;
        b=kuQaKUk+QL4F9aO2SZZXt/TZUmzzLGxO/qD5FAEaLxiigWF0iPhkFplGRAAx4FKwnr
         Wlb4TzDwG1DMTJ8ieLDzaobDNixqLyIaZ7PNe05OKtrWJaKq0eC2y5MYYB9V6uEOy8Wi
         DDJKXXLD95XS41rmi/K4uOfpBq8SgwHSewd9Q=
MIME-Version: 1.0
In-Reply-To: <427155180906111316n5a81fb81x132093fc76f8e259@mail.gmail.com>
References: <4A2D79C8.7050705@hanik.com> <4A313324.6080907@hanik.com>
	 <427155180906111204m7e2c1f15k398ace65de28ffa6@mail.gmail.com>
	 <427155180906111211o23ad4b63iedc2d5c05b023add@mail.gmail.com>
	 <4A316021.1060205@hanik.com>
	 <427155180906111316n5a81fb81x132093fc76f8e259@mail.gmail.com>
Date: Fri, 12 Jun 2009 05:28:25 +0100
Message-ID: <25aac9fc0906112128q418fc4d2ke15353151f32287c@mail.gmail.com>
Subject: Re: [VOTE] Release JDBC Pool module v1.0.3
From: sebb <sebbaz@gmail.com>
To: Tomcat Developers List <dev@tomcat.apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

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.

==

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.

Also SlowQueryReport does not have a threshold attribute.
There may be other inconsistencies; I did not check it all

The Copyright years in the html files are wrong.

==

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.

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.

It's confusing to have two classes called ConnectionPool (or is this
required by JMX/MBean?)

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