db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernt M. Johnsen" <Bernt.John...@Sun.COM>
Subject Re: [PATCH] (DERBY-104) Get rid of the Max lenght of 18 for constraint names
Date Thu, 12 May 2005 00:15:24 GMT
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.

-- 
Bernt Marius Johnsen, Database Technology Group, Sun Microsystems, Norway

Mime
View raw message