See comments inline.
Army wrote (2005-05-11 16:18:42):
Bernt M. Johnsen wrote:
Hi all,
Could somebody review this patch (my first)? Now all identifiers
should have a max length of 128 (note: MAX_USERID_LENGTH is
unchanged).
I reviewed this patch and it looks good to me. I noticed that in the JIRA
entry for this bug, you had written that you wanted to do the following:
[ begin quote ]
2) Ensure that all DB2 related constants are prefixed by DB2_
There are now 6 constants which do not have the prefix:
MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA,
MAX_USERID_LENGTH, MAX_DECIMAL_PRECISION_SCALE, DEFAULT_DECIMAL_PRECISION,
DEFAULT_DECIMAL_SCALE
These should get the DB2_ prefix if they are DB2-related (are they?)
[ end quote ]
Did you decide to not do this part?
I never got an answer, so I decided not to touch these constants.
For the record, the comments in the
code suggest that the first three of these
(MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, and
MAX_USERID_LENGTH) are definitely DB2-related, so adding the "DB2_" prefix
to those would make sense.
Should I add "DB2_"-prefix to these constants, then? (and leave the
latter-three unchanged?).
As for the latter three, one might assume that
they are DB2-specific since they were declared in the "DB2Limits.java"
file, but maybe that's not a valid assumption...anyone out there know?? On
the one hand, I think these "DB2_" prefix changes could easily be checked
in later with a different patch, if someone wants to do it. On the other,
it might be less inuitive to people browsing changes to see a solo patch
that adds a "DB2_" prefix to some constants in "Limits.java"--at the very
least, the patch to do so would have to be very well-commented, or else it
could lead to confusion.
NOTE: I couldn't get svn diff to reflect that I have renamed one file
(done with svn rename). If it is a problem, I can undo the rename and
submit a new patch.
I did have problems applying the patch to the my local codeline, and I'm
pretty sure it was because of this. In order to get the patch to apply, I
manually copied "DB2Limit.java" to "Limits.java"--after doing that (and
only that), I was then able to successfully apply the patch. So whoever
commits this might need to do the same, or else use some other trick to get
the patch to apply correctly.
Seems to me that this is a weakness in the way we submit pathces
(probably a weakness in svn).
After applying the patch I ran a couple of the tests that were affected,
and they passed as expected.
One last question: did you have a chance to run the "derbyall" suite on
this to make sure you caught all of the relevant tests? I'm guessing that
you did based on the number of masters that you updated, but it'd be nice
to confirm--typically when you submit a patch, it's good to indicate what
tests you ran (and on what platforms/JVMs) just so that reviewers know you
actually tested your patch before posting it (which you clearly
did!) ;)
Yes, I ran derbyall:
Java Version: 1.4.2_02
Java Vendor: Sun Microsystems Inc.
OS name: Linux
OS architecture: i386
OS version: 2.4.20-31.9
All in all, looks good to me.
Thanks.
