Army wrote:
Bryan Pendleton updated DERBY-125:
----------------------------------
Attachment: changes2.html
svn-dec_28_2005.diff
[ snip ]
I think these changes are now fully ready for review. Please let me
know your comments as I am very interested to hear what you think.
I plan to review this patch, hopefully by the end of tomorrow
(Wednesday).
Well I'm a day late, but I've finished my review of the patch...
After reading through the (excellent) description of the changes that you wrote
and comparing your notes with my own understanding of the protocol, I couldn't
see anything wrong with your server changes. I ran both the derbynetmats and
the derbynetclientmats suite with your patch, and both suites ran clean. I also
ran some ODBC versions of the repros using the DB2 ODBC client (which is how I
discovered all of these issues in the first place), and it appears that all went
well there, too.
So I happily composed an email saying "looks good to me", and then, just before
sending the email off, I decided to try running your new test cases withOUT your
server changes--and that's where I came across some questions...
1) DERBY-491 test case (in procedure.java)
For the test for DERBY-491, we print a failure message if the retrieved string
doesn't match the expected string; else we don't print anything. However, when
I tried to run the test without your changes applied, this particular test case
still passed. So I'm wondering what part of your patch this test case (the
check for "s.equals(expected)") is actually testing? Is this related to the
off-by-one error that you described for DDMWriter (for DERBY-125)? In your
document, you wrote:
"The current code is missing the + 1, which means that we are starting one byte
too early [...] This problem manifests itself as a very tiny corruption of the
large object's data."
Is the "s.equals(expected)" in your test case used to check for this "tiny
corruption"? If so, then it definitely seems like this check _should_ fail
without your changes--but apparently that's not the case. So now I'm confused
as to a) what this part of the test is actually testing, and b) why we are not
(and haven't been) seeing incorrect results due to this off-by-one error, even
when your patch is not applied...?
In any event, I think it might be helpful to add more comments to this
particular part of the test, since it's not obvious how the
actual-vs-expected-string test is related to the error reported for DERBY-491.
For more (minor) comments on this test case, see the "Additional minor comments"
section below...
2) DERBY-170 test case (in prepStmt.java)
Without your changes, this test case still passes with the Derby Client (though
it fails with JCC). Is this expected? Does DERBY-170 only manifest itself with
JCC? That might very well be the case--and if so, you needn't worry about this
anymore--but I just thought I'd ask.
Note that this failure also shows up with the DB2 ODBC client, and with your
patch applied, it no longer occurs--so I think the fix is correct. I just want
to confirm that, even without your patch, the test isn't expected to fail for
Derby Client.
3) DERBY-125 test case (in prepStmt.java)
When I ran the DERBY-125 test case without your changes, it still passes, both
with JCC and the Derby Client. I guess this isn't surprising since, as your
comment for DERBY-125 says, "there is no exception or other 'hard' evidence of
the problem"--at least, not with JCC nor Derby Client (the DB2 ODBC client will
fail because of it).
In the ideal world, this test case would fail without your patch and succeed
with it, so that if this area of code is changed, we can easily catch any
regressions. Is there a (clean) way to do that with your changes? For example,
is there some condition in the server code for which we can check and then
throw, say, an assertion failure if it is not met? In your test case you
noticed that, before your patch, the server was writing a SQLDARD message that
was 118,300+ bytes long, which (I think?) was the cause of the problem. Is
there some place where we can check for this kind of thing in the server code
and fail (on the server side) if we find it? Then your test case for DERBY-125
would fail without your patch, which is the desired behavior. Of course, if you
can think of a better way to achieve the same result, feel free to go with what
you think is best.
--------------------------
Additional minor comments
--------------------------
= 0 = General comment.
I think that, as a general rule, it's nice if patches for different issues can
be checked in separately, instead of as a single patch like this one. That
makes it easier to track the changes/fixes, and in the event that a change for
one issue needs to be backed out for whatever reason, the other changes for
other issues can still remain in the codeline. That said, I'm not asking you to
break this particular patch up--I'm just giving a suggestion for future patches.
In this case, I think the changes are all very much inter-related, as you
describe in your document, so I personally am okay with having a single patch.
If anyone out there feels like you should break this patch up into four separate
patches (one for each issue being fixed), then I'll leave it up to him/her to
post saying so...
On a different note, it looks like the indentation for some of the changes
you've made differ from the surrounding code. I know that people on the lists
have said they prefer spaces over tabs, and that's fine with me if one is adding
a new class or method--but in cases where one is adding code to existing blocks,
I think it's cleaner to mimic the indentation of the block that's being
modified. For example, in DDMReader.java:
@@ -1710,8 +1712,11 @@
"fill",
5);
}
- count += actualBytesRead;
- totalBytesRead += actualBytesRead;
+ if (actualBytesRead != -1)
+ {
+ count += actualBytesRead;
+ totalBytesRead += actualBytesRead;
+ }
}
The lines that were deleted, along with the rest of the code in this procedure,
all begin with tabs; but the lines that were added with this patch all begin
with spaces, which makes the format of the above code a bit awkward. I think
it'd be nice if the new lines matched the indentation of the existing lines...
----
= 1 = procedure.java
I think it would be good to add a try-catch block around the do-while loop,
since the error reported for DERBY-491 was a protocol exception. Then in the
catch block, print an explicit error saying that DERBY-491 failed with an
exception; this clarifies where in the new code we're testing the problem
described in DERBY-491, and it will also help debugging in the future if
something breaks your fix. So I'd say do something along the lines of (this is
untested code):
cSt.setInt(1, 3);
try {
cSt.execute();
String expected = new String(cData);
boolean success = true;
do {
ResultSet rs = cSt.getResultSet();
while (rs.next()) {
String s = rs.getString(2);
if (!s.equals(expected)) {
System.out.println("JIRA-491 FAILURE: " + s + " is not " +
expected);
success = false;
}
}
} while (cSt.getMoreResults());
if (success)
System.out.println("JIRA-491 successful.");
} catch (Exception e) {
System.out.println("JIRA-491 FAILURE: Caugh exception:");
e.printStackTrace(System.out);
}
This approach will also allow the JIRA-492 test to execute even if the test for
JIRA-491 fails; currently, if JIRA-491 fails with a protocol exception, we'll
exit the method before JIRA-492 is run.
----
= 2 = procedure.java
Could we just remove the following line since it's commented out, or is it there
for a reason?
+ //cSt.registerOutParameter(1, Types.INTEGER);
----
= 3 = procedure.java
There are a couple of places where we have things like:
+ } catch (SQLException se) { System.out.println(se.getSQLState()); }
I realize these lines came from the repro I posted, but for purposes of these
tests, I think it'd be good to add some kind of indication that these sqlstates
are NOT expected. A tag like "FAILED: UNEXPECTED SQLException: " (and then
print SQLException) is more likely to catch someone's eye and make them
double-check their work.
----
= 5 = prepStmt.java
+ if (e.getMessage().indexOf("JIRA170") > 0)
+ System.out.println("Jira170: caught expected table not found");
I think it'd be safer to check the SQLState of the error message, instead of
searching for a specific string:
if (e.getSQLState().equals(SQLState.LANG_TABLE_NOT_FOUND))
System.out.println("...");
--------
Overall, the code changes you've made look good to me, and the explanations that
you gave for your changes are both thorough and convincing--and, so far as I can
tell, correct :) The tests (derbynetmats, derbynetclientmats, and some ODBC
tests that I ran) all seem to agree.
So in short, if you can address the comments above re: the test cases that
you've added, I think this patch will be ready for commit.
Thanks for the excellent investigation, explanation, and code changes. I think
this will be a _great_ patch for Derby Network Server once it's finalized: not
only will it fix several current server issues, but it will also help to make
the server much more reliable when dealing with large data flow in general. So
thanks for the hard work!
Army
