db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From TomohitoNakayama <tomon...@basil.ocn.ne.jp>
Subject Re: [jira] Commented: (DERBY-1471) Implement layer B streaming for new methods defined in JDBC4.0
Date Thu, 07 Dec 2006 13:31:56 GMT
Ooops.

>> * In NetStatementRequest, 0x8002 is used as a magic number many
>>  places. Perhaps it could be explained in a comment, or a constant
>>  could be used instead?
>>
> This number comes from specification of DRDA.
> It told to set the Layer B 2-byte object length field to X'8004'.
>
> I think comment is needed in the code.


We have both 0x8002 and 0x8004 in the patch.
Either of them need comment ....

Best regards.


TomohitoNakayama wrote:

> Thank you for your attention.
>
> I think another patch is needed before committing ...
>
>> * BrokeredPreparedStatement now implements length-less setBinaryStream
>>  and setCharacterStream. We could therefore remove them from
>>  BrokeredPreparedStatement40.
>>
>> * In DRDAConnThread.readAndSetExtParam(), some of the code has been
>>  changed from
>>
>>    if (stream == null) {
>>       ...
>>    } else {
>>       ...
>>    }
>>
>>  to
>>
>>    if (stream == null) {
>>        ...
>>    } else if (stream instanceof StandardEXTDTAReaderInputStream) {
>>        ... // basically do the same as in the old else clause
>>    } else if (stream instanceof LayerBStreamedEXTDTAReaderInputStream) {
>>        ...
>>    }
>>
>>  I would feel more comfortable if the code in the first else-if
>>  clause were put into an else clause. If the stream is of another
>>  type (shouldn't happen, but someone might extend the code later), it
>>  will be silently ignored.
>>
> I see.
> I will fix those in next patch.
>
>
>> * In NetStatementRequest, 0x8002 is used as a magic number many
>>  places. Perhaps it could be explained in a comment, or a constant
>>  could be used instead?
>>
> This number comes from specification of DRDA.
> It told to set the Layer B 2-byte object length field to X'8004'.
>
> I think comment is needed in the code.
>
>
>> * Inner class PublicBufferOutputStream in DRDAConnThread could be
>>  private. Or even better, since ReEncodedInputStream also implements
>>  such a class, it could be implemented as a stand-alone class that
>>  could be shared between DRDAConnThread and ReEncodedInputStream.
>>
> I think PublicBufferOutputStream should not be shared between classes,
> because accessing internal buffer is a kind of a necessary evil and
> should not be used widely ...
>
> Well ... I will make it as private.
>
>
>> * Blob/Clob: The variable willBeLayerBStreamed_ and the method
>>  willBeLayerBStreamed() could be moved to the base class (Lob) to
>>  avoid duplicated code.
>>
> I am somewhat repulsed for sharing code around instance variable ....
>
> However, the value never be changed in the life of the object and
> they are equal in semantics ....
>
> I will try to modify the code and will judge the answer reading the 
> result.
>
>
> Best regards.
>
>
> Knut Anders Hatlen (JIRA) wrote:
>
>>    [ 
>> http://issues.apache.org/jira/browse/DERBY-1471?page=comments#action_12456093 
>> ]            Knut Anders Hatlen commented on DERBY-1471:
>> -------------------------------------------
>>
>> I have looked at patch 3. I don't know enough about layer B streaming
>> to say whether all details of the implementation are correct, but I
>> have a couple of comments. All of the comments are about minor issues,
>> so it would be perfectly OK to commit the patch as it is and address
>> the comments in a followup patch.
>>
>> * BrokeredPreparedStatement now implements length-less setBinaryStream
>>  and setCharacterStream. We could therefore remove them from
>>  BrokeredPreparedStatement40.
>>
>> * In DRDAConnThread.readAndSetExtParam(), some of the code has been
>>  changed from
>>
>>    if (stream == null) {
>>       ...
>>    } else {
>>       ...
>>    }
>>
>>  to
>>
>>    if (stream == null) {
>>        ...
>>    } else if (stream instanceof StandardEXTDTAReaderInputStream) {
>>        ... // basically do the same as in the old else clause
>>    } else if (stream instanceof LayerBStreamedEXTDTAReaderInputStream) {
>>        ...
>>    }
>>
>>  I would feel more comfortable if the code in the first else-if
>>  clause were put into an else clause. If the stream is of another
>>  type (shouldn't happen, but someone might extend the code later), it
>>  will be silently ignored.
>>
>> * Inner class PublicBufferOutputStream in DRDAConnThread could be
>>  private. Or even better, since ReEncodedInputStream also implements
>>  such a class, it could be implemented as a stand-alone class that
>>  could be shared between DRDAConnThread and ReEncodedInputStream.
>>
>> * In NetStatementRequest, 0x8002 is used as a magic number many
>>  places. Perhaps it could be explained in a comment, or a constant
>>  could be used instead?
>>
>> * Blob/Clob: The variable willBeLayerBStreamed_ and the method
>>  willBeLayerBStreamed() could be moved to the base class (Lob) to
>>  avoid duplicated code.
>>
>>  
>>
>>> Implement layer B streaming for new methods defined in JDBC4.0
>>> --------------------------------------------------------------
>>>
>>>                Key: DERBY-1471
>>>                URL: http://issues.apache.org/jira/browse/DERBY-1471
>>>            Project: Derby
>>>         Issue Type: New Feature
>>>         Components: Network Client
>>>           Reporter: Tomohito Nakayama
>>>        Assigned To: Tomohito Nakayama
>>>        Attachments: DERBY-1471.diff, DERBY-1471.patch, 
>>> DERBY-1471.stat, DERBY-1471_2.patch, DERBY-1471_2.stat, 
>>> DERBY-1471_3.patch, DERBY-1471_3.stat
>>>
>>>
>>> JDBC 4.0 introduced new methods which take parameters for object to 
>>> be sent to sever without length information.
>>> For those methods, Layer B streaming is best way to implement 
>>> sending object to server.
>>> This issue is representation of DERBY-1417 in Network Client.
>>>   
>>
>>
>>  
>>
>

-- 
/*

        Tomohito Nakayama
        tomonaka@basil.ocn.ne.jp
        tomohito@rose.zero.ad.jp
        tmnk@apache.org

        Naka
        http://www5.ocn.ne.jp/~tomohito/TopPage.html

*/ 


Mime
View raw message