db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bryan Pendleton <bpendle...@amberpoint.com>
Subject Re: [jira] Updated: (DERBY-326) Improve streaming of large objects for network server and client
Date Thu, 05 Jan 2006 00:55:46 GMT
TomohitoNakayama wrote:
> Hello.
> I have uploaded DERBY-326_3.patch.

Thank you very much for considering my comments.

I think that your most recent patch (DERBY-326_3) is quite good,
but I have a few more thoughts that I wanted to share.



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;

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;
       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.

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()

         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.

View raw message