Quick clarification on my last point:
createShardingKeyBuilder() is not guaranteed to be implemented ... even
with sharding supported. This method is documented to be optional; it does
not contain the usual "if sharding is not supported" warning but "if the
driver does not support this method", which is a different condition.
That's why I think the examples could be misleading.
Cheers,
Paul
On Tue, Nov 24, 2015 at 9:31 AM, Paul Benedict <pbened...@apache.org> wrote:
> 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.ander...@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.w...@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.ander...@oracle.com
>> >>
>> >>
>> >>
>> >
>>
>>
>>
>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering
>> 1 Network Drive
>> Burlington, MA 01803
>> lance.ander...@oracle.com
>>
>>
>>
>>
>
