Review request for 8001536

[Lance Andersen - Oracle]

Hi Remi,


Thank you for the feedback

On Oct 30, 2012, at 2:05 PM, Remi Forax wrote:

> On 10/30/2012 05:25 PM, Lance Andersen - Oracle wrote:
>> Hi,
>> 
>> This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/.  This adds read/writeObject as well as clone methods to SerialXLob classes.
>> 
>> All SQE tests passed,  1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address.  JDBC Unit tests all pass .
> 
> Hi Lance.
> In SerialBlob and in SerialClob
> test (obj == null) is not necessary in equals, null instanceof X is always false.

OK, thanks for the suggestion, I will make this change
> 
> in hashCode, Objects.hash() allocate an array to pass arguments to Arrays.hashCode() and box primitive values to Object.
> while this method is really convenient to use, each calls will allocate an array and box the two values,
> the overhead seems to high here.
> This code should be equivalent:
>    return ((31 +Arrays.hashCode(buf)) * 31 +len) * 31 + origLen;
I can simplify hashCode to the what you have above, I liked the convenience method which is why I was using it. But happy to change it
> 
> in clone, sb should not be initialized to null

I think it is OK as I have it as  HashMap does it similar to what I have done vs ArrayList which follows your suggestion.  Do we have a preferred practice or is this just a style choice?
> and the catch should be: throw new InternalError(e),

Given I am providing clone(), I did not see a reason to provide InternalError().  I have no strong preference but some java classes do and others do not (HashMap for example), so what is our preferred standard?
> this is the standard code you can see in clone.
> 
> in readObject, the test (buf.length != len) can be done before decoding the blob.

True, I can move it up.
> 
> in writeObject, you set "blob" twice, which is weird,

my bad, I forgot to remove that.
> also I think that if blob is not Serializable,
> the code should throw an exception, so you should not use instanceof and let s.writeFields()
> to throw NotSerializable exception.

This is intentional.  A Blob or Clob will not be serializable as its properties are unique to the database and it is created from an active Connection object.  

In the event someone actually tried to serialize this, I do not want it to fail just because someone passed in a LOB to instantiate this beast (note these methods should never have been created this way but this is way before my time).

In the unlikely event someone created their own wrapped Blob/Clob (which I cannot see why anyone would do), I am allowing for both for backwards compatibility.

Best
Lance
> 
> cheers,
> Rémi
> 
>> 
>> 
>> 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
>> 
> 

-------------- next part --------------

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