RFR: 8085984 Add JDBC Sharding API

Paul Benedict pbenedict at apache.org
Tue Nov 24 15:31:45 UTC 2015


Some comments:

General
*) Shouldn't the javadocs be put mentions of null in {@code null} tags?
*) Many methods are documented to throw SQLFeatureNotSupportedException
when sharding isn't supported, but the error message actually says "XYZ not
implemented". Well I think that places an immediate burden on the driver
implementors to correct the error message. If it's simply not supported by
default, the message should simply be "XYZ does not support sharding" (or
something similar) rather than talk about lack of implementation. WDYT?

XADataSource
*) createXAConnectionBuilder needs its error message fixed. Add "XA" in the
string
*) I noticed the closing method braces don't align with "default" keyword

ConnectionBuilder/XAConnectionBuilder
*) shardingKey() and superShardingKey() could benefit from a {@see} to
ShardingKeyBuilder since ShardingKeyBuilder actually explains what a
[super]sharding key is. I actually prefer the explanation of what these
keys are to be copied into these methods, but a {@see} would be the minimal.

ShardingKey/ShardingKeyBuilder/ConnectionBuilder/XAConnectionBuilder
*) All the examples use createShardingKeyBuilder() but that method is not
guaranteed to be implemented. Don't you think that might make the examples
misleading?

Cheers,
Paul

On Mon, Nov 23, 2015 at 7:21 PM, Lance Andersen <lance.andersen at oracle.com>
wrote:

> Hi Joe,
>
> Thank you and Ulf for catching the error message typo as we changed the
> name of the method.   I will fix that before I push
>
> Best
> Lance
> On Nov 23, 2015, at 7:09 PM, huizhe wang <huizhe.wang at oracle.com> wrote:
>
> > Hi Lance,
> >
> > Looks good, except as Ulf pointed out already, in both in
> Connection.java and XAConnection.java, SQLFeatureNotSupportedException
> should have been thrown with a message "setShardingKeyIfValid not
> implemented" rather than "isValid not implemented" (4 occurrences in total).
> >
> > Best,
> > Joe
> >
> > On 11/23/2015 3:01 PM, Lance Andersen wrote:
> >> Hi all,
> >>
> >> Need a reviewer for 8085984, which adds support for Sharding.  The CCC
> has been approved.  The webrev can be found at
> http://cr.openjdk.java.net/~lancea/8085984/webrev.00/
> >>
> >> Best,
> >> Lance
> >>
> >>
> >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> >> Oracle Java Engineering
> >> 1 Network Drive
> >> Burlington, MA 01803
> >> Lance.Andersen at oracle.com
> >>
> >>
> >>
> >
>
>
>
> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> Lance.Andersen at oracle.com
>
>
>
>



More information about the core-libs-dev mailing list