You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by Bryan Pendleton <bp...@amberpoint.com> on 2006/01/01 18:31:01 UTC
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Tomohito Nakayama (JIRA) wrote:
> I re-upload DERBY-326_2.patch.
> I created patch *after* applying patch created executing "svn diff --diff-cmd diff -x -uw".
Thank you very much for re-posting the patch without the whitespace
changes. It was much easier to read and understand this way. A few
comments below:
1) It was hard for me to understand the purpose of the class
ReEncodedInputStream.java simply from reading the code. I
think it would be good for you to add some comments describing
the purpose of this class and its expected uses.
2) It seems like it might be possible to improve these lines in
ReEncodedInputStream.java:
if(reader == null){
throw new NullPointerException();
}
if(enc == null){
throw new NullPointerException();
}
I think it would be better to pass a string to the exception
constructor, indicating which variable was null, as in:
throw new NullPointerException(
"null 'reader' passed to ReEncodedInputStream");
That way, if there are no line numbers available in the stack
trace, we'll still be able to tell the two exceptions apart.
3) In EXTDTAInputStream.getEXTDTAStream(), I did not understand
the purpose of the "is" InputStream variable. It seems to be
initialized to null, then checked in the finally{} block, but
is otherwise unused. Am I missing something? Or can this
variable be removed?
4) In EXTDTAInputStream.openInputStreamLazily, I think it would be
better to change the line
throw new IllegalStateException();
to
throw new IllegalStateException(
"Either 'blob' or 'clob' must be non-null");
5) Also in EXTDTAInputStream.openInputStreamLazily, what is supposed
to happen in the empty catch block for UnsupportedEncodingException?
Do you really intend just to ignore this exception? If so, then
I think it would be good to add some comments explaining why it
is appropriate to catch and ignore this exception.
6) Are the comments above DDMWriter.writeScalarStream() still
accurate? It looks like you have addressed some of these issues,
but you didn't seem to change the comment. It would be good to
check the comment and see if it needs to be updated.
7) It seems like the writeScalarStream method and its related methods
in DDMWriter.java (prepScalarStream, flushScalarStreamSegment, etc.)
do not use the "ensureLength()" protocol which is common elsewhere
in DDMWriter.java, but instead they examine the buffer length by
checking bytes.length and then being careful not to overrun that
size. I'm not sure exactly why these methods are different from all
the other data-writing methods in this respect, but I think it would
be nice to add some comments to the code describing this behavior.
Overall, I thought your changes were very good, and I think it will
be a definite improvement to avoid having to precompute the large
object length when returning it to the client.
thanks,
bryan
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by TomohitoNakayama <to...@basil.ocn.ne.jp>.
Happy new year :)
Thank you for your reviewing the patch.
I just started to improve the patch, based on your comment.
I will submit the new patch in days.
Please read it again.
Best regards.
Bryan Pendleton wrote:
> Tomohito Nakayama (JIRA) wrote:
>
>> I re-upload DERBY-326_2.patch.
>> I created patch *after* applying patch created executing "svn diff
>> --diff-cmd diff -x -uw".
>
>
> Thank you very much for re-posting the patch without the whitespace
> changes. It was much easier to read and understand this way. A few
> comments below:
>
> 1) It was hard for me to understand the purpose of the class
> ReEncodedInputStream.java simply from reading the code. I
> think it would be good for you to add some comments describing
> the purpose of this class and its expected uses.
>
> 2) It seems like it might be possible to improve these lines in
> ReEncodedInputStream.java:
>
> if(reader == null){
> throw new NullPointerException();
> }
>
> if(enc == null){
> throw new NullPointerException();
> }
>
> I think it would be better to pass a string to the exception
> constructor, indicating which variable was null, as in:
>
> throw new NullPointerException(
> "null 'reader' passed to ReEncodedInputStream");
>
> That way, if there are no line numbers available in the stack
> trace, we'll still be able to tell the two exceptions apart.
>
> 3) In EXTDTAInputStream.getEXTDTAStream(), I did not understand
> the purpose of the "is" InputStream variable. It seems to be
> initialized to null, then checked in the finally{} block, but
> is otherwise unused. Am I missing something? Or can this
> variable be removed?
>
> 4) In EXTDTAInputStream.openInputStreamLazily, I think it would be
> better to change the line
>
> throw new IllegalStateException();
> to
> throw new IllegalStateException(
> "Either 'blob' or 'clob' must be non-null");
>
> 5) Also in EXTDTAInputStream.openInputStreamLazily, what is supposed
> to happen in the empty catch block for UnsupportedEncodingException?
> Do you really intend just to ignore this exception? If so, then
> I think it would be good to add some comments explaining why it
> is appropriate to catch and ignore this exception.
>
> 6) Are the comments above DDMWriter.writeScalarStream() still
> accurate? It looks like you have addressed some of these issues,
> but you didn't seem to change the comment. It would be good to
> check the comment and see if it needs to be updated.
>
> 7) It seems like the writeScalarStream method and its related methods
> in DDMWriter.java (prepScalarStream, flushScalarStreamSegment, etc.)
> do not use the "ensureLength()" protocol which is common elsewhere
> in DDMWriter.java, but instead they examine the buffer length by
> checking bytes.length and then being careful not to overrun that
> size. I'm not sure exactly why these methods are different from all
> the other data-writing methods in this respect, but I think it would
> be nice to add some comments to the code describing this behavior.
>
> Overall, I thought your changes were very good, and I think it will
> be a definite improvement to avoid having to precompute the large
> object length when returning it to the client.
>
> thanks,
>
> bryan
>
>
>
>
--
/*
Tomohito Nakayama
tomonaka@basil.ocn.ne.jp
tomohito@rose.zero.ad.jp
tmnk@apache.org
Naka
http://www5.ocn.ne.jp/~tomohito/TopPage.html
*/
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by Bryan Pendleton <bp...@amberpoint.com>.
TomohitoNakayama wrote:
> Hello.
>
> I uploaded DERBY-326_4.patch.
> I response each comment of Bryan.
Thank you for the updated patch, and for considering
my comments. I think the patch is good and I have no
further comments.
I am interested in seeing the comments of others on the list.
thanks,
bryan
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by TomohitoNakayama <to...@basil.ocn.ne.jp>.
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
*/
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by TomohitoNakayama <to...@basil.ocn.ne.jp>.
Hello Bryan.
Thank you very much.
Your conscientious review helps me a lot.
I will upload the new patch in days.
Please read it again.
Best regards.
Bryan Pendleton wrote:
> 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.
>
> thanks,
>
> bryan
--
/*
Tomohito Nakayama
tomonaka@basil.ocn.ne.jp
tomohito@rose.zero.ad.jp
tmnk@apache.org
Naka
http://www5.ocn.ne.jp/~tomohito/TopPage.html
*/
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by Bryan Pendleton <bp...@amberpoint.com>.
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.
thanks,
bryan
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;
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.
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.
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by TomohitoNakayama <to...@basil.ocn.ne.jp>.
Hello.
I have uploaded DERBY-326_3.patch.
Next are response for each comments of Bryan.
Bryan Pendleton wrote:
> 1) It was hard for me to understand the purpose of the class
> ReEncodedInputStream.java simply from reading the code. I
> think it would be good for you to add some comments describing
> the purpose of this class and its expected uses.
I have added documentation comment.
Please confirm it.
> 2) It seems like it might be possible to improve these lines in
> ReEncodedInputStream.java:
>
> if(reader == null){
> throw new NullPointerException();
> }
>
> if(enc == null){
> throw new NullPointerException();
> }
>
> I think it would be better to pass a string to the exception
> constructor, indicating which variable was null, as in:
>
> throw new NullPointerException(
> "null 'reader' passed to ReEncodedInputStream");
>
> That way, if there are no line numbers available in the stack
> trace, we'll still be able to tell the two exceptions apart.
>
I have added those message to exception.
> 3) In EXTDTAInputStream.getEXTDTAStream(), I did not understand
> the purpose of the "is" InputStream variable. It seems to be
> initialized to null, then checked in the finally{} block, but
> is otherwise unused. Am I missing something? Or can this
> variable be removed?
>
Thank you.
They were unused variables written in the middle of coding and testing.
I removed them.
> 4) In EXTDTAInputStream.openInputStreamLazily, I think it would be
> better to change the line
>
> throw new IllegalStateException();
> to
> throw new IllegalStateException(
> "Either 'blob' or 'clob' must be non-null");
>
I have added those information to the exception.
> 5) Also in EXTDTAInputStream.openInputStreamLazily, what is supposed
> to happen in the empty catch block for UnsupportedEncodingException?
> Do you really intend just to ignore this exception? If so, then
> I think it would be good to add some comments explaining why it
> is appropriate to catch and ignore this exception.
>
I have added comment to catch block of UnsupportedEncodingException.
> 6) Are the comments above DDMWriter.writeScalarStream() still
> accurate? It looks like you have addressed some of these issues,
> but you didn't seem to change the comment. It would be good to
> check the comment and see if it needs to be updated.
Thank you ...
I read the comment and take some important information.
Issues of ToDo Comment here was finished at server side.
However, comment says that there exists similar problem at client.
I have not touched client side code yet and want to leave it for another
patch.
I moved the comment to client side.
> 7) It seems like the writeScalarStream method and its related methods
> in DDMWriter.java (prepScalarStream, flushScalarStreamSegment, etc.)
> do not use the "ensureLength()" protocol which is common elsewhere
> in DDMWriter.java, but instead they examine the buffer length by
> checking bytes.length and then being careful not to overrun that
> size. I'm not sure exactly why these methods are different from all
> the other data-writing methods in this respect, but I think it would
> be nice to add some comments to the code describing this behavior.
The ensureLength() method was not used in original.
I don't know why.
However, reading the code around, I found that size of buffer can be
very small depending on parameters for constructor and,
call to ensureLength will adjust the size of buffer appropriate.
I added call to ensureLength() method in writeScalarStream().
Please read and review the patch again.
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
*/
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by TomohitoNakayama <to...@basil.ocn.ne.jp>.
Hello.
I took your comment that it is preferable not to use assert method,
if exception will happen without assert method.
I will work along it.
Best regards.
Daniel John Debrunner wrote:
>Those look ok, sometimes I get concerned about overuse of
>SanityManager/debug code that can obscure the actual code.
>With checking for null in sanity blocks, sometimes I see it as not worth
>it, e.g.
>
>
>Dan.
>
>
--
/*
Tomohito Nakayama
tomonaka@basil.ocn.ne.jp
tomohito@rose.zero.ad.jp
tmnk@apache.org
Naka
http://www5.ocn.ne.jp/~tomohito/TopPage.html
*/
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by Daniel John Debrunner <dj...@apache.org>.
TomohitoNakayama wrote:
> Hello.
> Daniel and David.
>
> Thank you.
> Two of your comments recalled me use of SanityManager.
>
> The modifications shown in next are relating part of the new patch.
>
> First part:
> + if(SanityManager.DEBUG){
> + SanityManager.ASSERT( blob != null ||
> + clob != null,
> + "Either blob or clob must be non-null.");
> + }
> +
> + if(blob != null){
> + is = blob.getBinaryStream();
> + + }else if(clob != null){
> + is = new ReEncodedInputStream(clob.getCharacterStream(),
> + NetworkServerControlImpl.DEFAULT_ENCODING);
>
<snip>
> Using SanityManager, overhead of tests can be removed.
Those look ok, sometimes I get concerned about overuse of
SanityManager/debug code that can obscure the actual code.
With checking for null in sanity blocks, sometimes I see it as not worth
it, e.g.
String s = somemethod();
if (SanityManager.DEBUG)
{
if (s == null)
SanityManager.THROWASSERT(" OH NO s is null");
}
return s.length();
My contention for code like this that the sanity check is not work it
and dominates the real code, focuses the eye on the check rather than
the actual code. Since s.length() will throw a NPE if s is null, the
check is already built into the Java language.
This, in my opinion, is much cleaner.
return somemethod().length();
and during development time, any NPE can be resolved through the
information in the stack trace or use of a debugger.
Similar for other checks, like sanity instanceof
Object o = somemethod();
if (SanityManager.DEBUG)
{
if (o != null && !(o instanceof org.apache.derby.some.clazz))
SanityManager.THROWASSERT(" OH NO o is not a
org.apache.derby.some.clazz it is a " + o.getClass());
}
return (org.apache.derby.some.clazz) o;
much cleaner:
return (org.apache.derby.some.clazz) somemethod();
Dan.
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by TomohitoNakayama <to...@basil.ocn.ne.jp>.
Hello.
Daniel and David.
Thank you.
Two of your comments recalled me use of SanityManager.
The modifications shown in next are relating part of the new patch.
First part:
+ if(SanityManager.DEBUG){
+ SanityManager.ASSERT( blob != null ||
+ clob != null,
+ "Either blob or clob must be non-null.");
+ }
+
+ if(blob != null){
+ is = blob.getBinaryStream();
+
+ }else if(clob != null){
+ is = new ReEncodedInputStream(clob.getCharacterStream(),
+ NetworkServerControlImpl.DEFAULT_ENCODING);
Second part:
+ public ReEncodedInputStream(Reader reader,String enc)
+ throws UnsupportedEncodingException,
+ IOException {
+
+ if(SanityManager.DEBUG){
+ SanityManager.ASSERT( reader != null,
+ "null 'reader' passed to ReEncodedInputStream" );
+ SanityManager.ASSERT( enc != null,
+ "null 'encoding' passed to ReEncodedInputStream" );
+ }
+
+ reader_ = reader;
+ enc_ = enc;
+ buffer_ = reEncode(reader_,
+ enc,
+ BUFFERED_CHAR_LEN);
+
+ }
Using SanityManager, overhead of tests can be removed.
Furthermore, placing these testing code as not used in usual operation,
it is not necessary for these messages to be internationalized.
// I knew name of assert, however, I have not used it ever.
// This was good experience for me :)
Best regards.
Daniel John Debrunner wrote:
>David W. Van Couvering wrote:
>
>
>
>>Bryan Pendleton wrote:
>>
>>
>>
>>>2) It seems like it might be possible to improve these lines in
>>> ReEncodedInputStream.java:
>>>
>>> if(reader == null){
>>> throw new NullPointerException();
>>> }
>>>
>>> if(enc == null){
>>> throw new NullPointerException();
>>> }
>>>
>>> I think it would be better to pass a string to the exception
>>> constructor, indicating which variable was null, as in:
>>>
>>> throw new NullPointerException(
>>> "null 'reader' passed to ReEncodedInputStream");
>>>
>>>
>
>Actually, I'd be interested to know why such tests are required? Why is
>there a need to throw a NPE if a parameter or variable is null?
>
>It's not a coding style I like.
>
>Dan.
>
>
>
>
>
--
/*
Tomohito Nakayama
tomonaka@basil.ocn.ne.jp
tomohito@rose.zero.ad.jp
tmnk@apache.org
Naka
http://www5.ocn.ne.jp/~tomohito/TopPage.html
*/
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by Mike Matrigali <mi...@sbcglobal.net>.
Yes, if it is a code bug case trying to be caught then using of
asserts to provide more info in debug mode is fine with me. And no
need to internationalize.
Daniel John Debrunner wrote:
> David W. Van Couvering wrote:
>
>
>>
>>Bryan Pendleton wrote:
>>
>>
>>>2) It seems like it might be possible to improve these lines in
>>> ReEncodedInputStream.java:
>>>
>>> if(reader == null){
>>> throw new NullPointerException();
>>> }
>>>
>>> if(enc == null){
>>> throw new NullPointerException();
>>> }
>>>
>>> I think it would be better to pass a string to the exception
>>> constructor, indicating which variable was null, as in:
>>>
>>> throw new NullPointerException(
>>> "null 'reader' passed to ReEncodedInputStream");
>>
>
> Actually, I'd be interested to know why such tests are required? Why is
> there a need to throw a NPE if a parameter or variable is null?
>
> It's not a coding style I like.
>
> Dan.
>
>
>
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects
for network server and client
Posted by Daniel John Debrunner <dj...@apache.org>.
David W. Van Couvering wrote:
>
>
> Bryan Pendleton wrote:
>
>>
>> 2) It seems like it might be possible to improve these lines in
>> ReEncodedInputStream.java:
>>
>> if(reader == null){
>> throw new NullPointerException();
>> }
>>
>> if(enc == null){
>> throw new NullPointerException();
>> }
>>
>> I think it would be better to pass a string to the exception
>> constructor, indicating which variable was null, as in:
>>
>> throw new NullPointerException(
>> "null 'reader' passed to ReEncodedInputStream");
>
Actually, I'd be interested to know why such tests are required? Why is
there a need to throw a NPE if a parameter or variable is null?
It's not a coding style I like.
Dan.
Re: [jira] Updated: (DERBY-326) Improve streaming of large objects for
network server and client
Posted by "David W. Van Couvering" <Da...@Sun.COM>.
Bryan Pendleton wrote:
>
> 2) It seems like it might be possible to improve these lines in
> ReEncodedInputStream.java:
>
> if(reader == null){
> throw new NullPointerException();
> }
>
> if(enc == null){
> throw new NullPointerException();
> }
>
> I think it would be better to pass a string to the exception
> constructor, indicating which variable was null, as in:
>
> throw new NullPointerException(
> "null 'reader' passed to ReEncodedInputStream");
>
> 4) In EXTDTAInputStream.openInputStreamLazily, I think it would be
> better to change the line
>
> throw new IllegalStateException();
> to
> throw new IllegalStateException(
> "Either 'blob' or 'clob' must be non-null");
>
I do hope that these strings are internationalized rather than using
hardcoded English... :)