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] Updated: (DERBY-326) Improve streaming of large objects for network server and client
Date Sat, 07 Jan 2006 12:39:23 GMT
Hello.

I uploaded DERBY-326_4.patch.
I response each comment of Bryan.

Bryan Pendleton wrote:

> 1) It seems that DDMWriter.writeScalarStream is
> defined to return an integer return code, and the return
> value appears to be the number of bytes in the stream
> that it wrote.
>
> However, I don't think that the callers of this method
> check the return code. At least, DRDAConnThread appears
> to discard the value returned from writeScalarStream, and
> I think it is the only caller of this method.
>
> I think that it would be an improvement to make this
> method be a 'void' method rather than returning an int.
>
> In addition to changing the return type to 'void', I think
> that we could also remove the following code at the end
> of writeScalarStream(). I'm not quite sure what this code
> is attempting to do, but if we don't care what the return
> value is, then I believe that we don't need this code either:
>
> // check to make sure that the specified length wasn't too small
> try {
> if (in.read() != -1) {
> totalBytesRead += 1;
> }
> }
> catch (java.io.IOException e) {
> // Encountered error in stream length verification for
> // InputStream, parameter #" + parameterIndex + ".
> // Don't think we need to error for this condition
> }
> return totalBytesRead;
>
I modified return type of writeScalarStream as void, and removed the
code mentioned.


> 2) This is not terribly important, but it seems like you could
> simplify the end of DDMWriter.prepScalarStream a bit, by using
> the 'nullIndicatorSize' variable. Instead of:
>
> if (writeNullByte)
> return DssConstants.MAX_DSS_LENGTH - 6 - 4 - 1 - extendedLengthByteCount;
> else
> return DssConstants.MAX_DSS_LENGTH - 6 - 4 - extendedLengthByteCount;
>
> I think you could just write:
>
> return DssConstants.MAX_DSS_LENGTH - 6 - 4 - nullIndicatorSize -
> extendedLengthByteCount;
>
> I leave it to you as to whether you think that is an improvement or not.
>
I modified the use of nullIndicatorSize.
Please confirm it.

> 3) At the top of DDMWriter.writeScalarStream(), it might be nice to
> add a short comment in front of these lines, indicating that the
> reason we need to do this here is so that we can call peekStream()
> later:
>
> if( !in.markSupported() )
> in = new BufferedInputStream(in);
>
> Perhaps something like:
>
> // If the stream that was passed in cannot be marked and reset,
> // then we need to wrap it in a BufferedInputStream so that
> // we can call peekStream() while processing it.
>
I added the comment.
Please read it.


Furthermore, I added next two modification in this patch.
1: If length of stream was known, new patch call ensureLength with that
information of length.
2: Move the call of close method of Stream to finally block, in
DRDAConnThread.


Best regards.

-- 
/*

        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