db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Army <qoz...@sbcglobal.net>
Subject Re: [PATCH] (DERBY-104) Get rid of the Max lenght of 18 for constraint names
Date Wed, 11 May 2005 23:18:42 GMT
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: 
These should get the DB2_ prefix if they are DB2-related (are they?)

[ end quote ]

Did you decide to not do this part?  For the record, the comments in the code 
suggest that the first three of these (MIN_COL_LENGTH_FOR_CURRENT_USER, 
DB2-related, so adding the "DB2_" prefix to those would make sense.  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.

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!) ;)

All in all, looks good to me.

View raw message