> On April 15, 2016, 1:52 p.m., Nate Cole wrote:
> > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql, lines 181-184
> > <https://reviews.apache.org/r/46269/diff/1/?file=1346529#file1346529line181>
> >
> >     Let's be consistent.  Either:
> >     pk_sc_desired_state
> >     fk_scds_...
> >     unq_scdesired...
> >     
> >     or
> >     PK_sc_desired
> >     FK_scds...
> >     UNQ_scdesired...
> >     
> >     I seem recall issues of case between the sql files and UpgradeCatalog* 
> > classes.  Jonathan?
> 
> Jonathan Hurley wrote:
>     I'm all for consistency. You're right that we used to have problems with 
> the case of the contraints. For PK's, we added some utility methods which 
> don't require us to know the name anymore:
>     
> https://github.com/apache/ambari/blob/trunk/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessor.java#L569-L581
>     
>     We could do something similar for FKs as well.
> 
> Balázs Bence Sári wrote:
>     I've got a couple of questions:
>     - The method in DBAccessor only works with 3 databases. Can we rely on 
> that?
>     - I just learned that the name of the constraints are important during 
> upgrade. So I guess I'd need to revert any constraint renamings I've done and 
> only name constraints that had been unnamed so far. (pls. confirm)
>     - Existing constraints are mostly in PK_the_rest_is_lowecase format, but 
> some are in all lowercase format (also unique is sometimes noted as UQ and 
> sometimes UNQ). I always used the PK/FK/UQ_the_rest_is_lowercase format 
> because that seemed to be the most frequent. Are you ok to follow that 
> convenions? (Also, what should happen with existing all-lowercase 
> constraints? Would renaming them break upgrade scripts?)
> 
> Nate Cole wrote:
>     I don't mind the PK/FK/UNQ_lower_name syntax myself, it's easy to 
> visually see "type" from "name" if you will.  These scripts are create-only, 
> so changing them shouldn't break existing deployments.  The concern is if 
> you've added new constraints, then yes, those need to be reflected in 
> UpgradeCatalog, but otherwise it seems safe.

I haven't introduced any new constraints, only renamed existing ones.
I am still a bit worried about renaming existing constraints (even naming 
unnamed ones). While these scripts are create only, there will still be 
inconsistency between fresh 2.4 installs and old installs upgraded to 2.4. It 
won't be an issue right away, but it can become one when we will upgrade to the 
next version after 2.4, unless we are 100% sure that after 2.4 upgrade catalogs 
will never drop any constraint by name (e.g. by using Jonathan's new API 
extended to all kinds of constraints).


- Balázs Bence


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46269/#review129114
-----------------------------------------------------------


On April 15, 2016, 1:41 p.m., Balázs Bence Sári wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46269/
> -----------------------------------------------------------
> 
> (Updated April 15, 2016, 1:41 p.m.)
> 
> 
> Review request for Ambari, Jonathan Robie, Nate Cole, and Nahappan 
> Somasundaram.
> 
> 
> Bugs: AMBARI-15915
>     https://issues.apache.org/jira/browse/AMBARI-15915
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> 1. Inlined alter table statements (except one to overcome a circular FK 
> dependency)
> 2. Reorganized create table statemens to avoid forward references
> 3. Named all unnamed constraints, shortened names to fit Oracle's 30 
> character limit
> 4. Unified constraint names across database flavors
> 5. Wrote a JUnit test that checks DDL's for consistency (all constraints are 
> named, the same tables and constraints are defined for all database flavors)
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql f90ac96 
>   ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 2b214c4 
>   ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql fc93372 
>   ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 870a8e8 
>   ambari-server/src/main/resources/Ambari-DDL-Postgres-EMBEDDED-CREATE.sql 
> 71d4813 
>   ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql 6e600c7 
>   ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql a3ea10d 
>   
> ambari-server/src/test/java/org/apache/ambari/server/orm/db/DDLTestUtils.java 
> PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/db/DDLTests.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46269/diff/
> 
> 
> Testing
> -------
> 
> 1. Run all java unit tests in ambari-server. All passed, except 
> ViewDirectoryWatcherTest.testDirectoryExtractionOnFileAdd(). Failure occured 
> to a hardcoded timeout value in the test being to low for my environment (I 
> raised the timeout from 7 secs to 20 and the test passed)
> 2. Run DDL's with their respective DB environment. In cases where the DDL 
> contained additional things other than table/index cration and insertion 
> (e.g. creating databases and users) I only run the table/index creation part.
> 
> 
> Thanks,
> 
> Balázs Bence Sári
> 
>

Reply via email to