> On Oct. 2, 2014, 11:29 p.m., Gwen Shapira wrote: > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java, > > line 869 > > <https://reviews.apache.org/r/26295/diff/1/?file=713258#file713258line869> > > > > If we add a unique constraint on job_name and link_name, we can just > > insert new job/link and catch the unique constraint exception. This will > > save us an extra query :) > > Abraham Elmahrek wrote: > I see what you're saying. In fact, I think we should have both: methods > to test the existance of a link and job and add the unique index. The > explanation of which will be provided momentarily... > > There are basically 3 things I think we should consider: > 1. Testability - It's easier to mock existsJob and existsLink than it is > to mock a DB. > 2. Error handling - Both designs provide some measure of error handling. > We throw our own SqoopException in both cases... but if we fail a constraint, > we'll have the DB error message to provide users as well. > 3. maintainability - existsLink and existsJob are re-usable. > > NOTE: I wouldn't put performance too high on this list since creating a > job/link will happen not so frequently. > > I think we should be using the method approach for the benefit of > testability and maintainability, but we should also have a unique constraint > to ensure database sanity. Would that work? If so, we should do the DB work > in a follow up Jira I think. Mainly because we'll have to do a bunch of > migration work. > > Veena Basavaraj wrote: > + 1, we should do it on configs too, if you are in that area. Since we > can now add ads and give config names for from/ to and driver. > + Please include that as well. I have more cosmetic suggestions following > > Veena Basavaraj wrote: > My + 1 was on Gwen's comment. > > Abe: I would rather disagree that having more code like this makes it > redable. It is less code with a DB constraint and more descroptive. I would > like it ot be on all tables. not just link/ jobs. ( connectors and configs as > well I meant) > > we shoudl decided which way is best before we do this, since it just adds > to more clean up later I think. > > Gwen Shapira wrote: > We don't take locks on the DB, so two jobs with identical names can end > in a race condition. So uniqueness is a must and so is handling the unique > exception correctly. General lesson - RDBMS were invented to maintain > consistency. Don't try to do it on your own at the app level. > > I don't mind leaving the linkExists checks for testability.
Hmm... I see value in what you guys are saying. How about this... let's do the other way first, then if we want to add cool error messages or something like that... we can tack that on later? - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26295/#review55277 ----------------------------------------------------------- On Oct. 2, 2014, 11:22 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26295/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2014, 11:22 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1477 > https://issues.apache.org/jira/browse/SQOOP-1477 > > > Repository: sqoop-SQOOP-1367 > > > Description > ------- > > commit 450db6146922bc197bb7c3eda375319788d610ba > Author: Abraham Elmahrek <[email protected]> > Date: Thu Oct 2 16:19:21 2014 -0700 > > SQOOP-1477: Sqoop2: Make link and job name unique identifier > > :100644 100644 3466116... e7d4589... M > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java > :100644 100644 a743491... 792306e... M > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > :100644 100644 54e37d9... 2489c58... M > core/src/main/java/org/apache/sqoop/repository/RepositoryError.java > :100644 100644 19b0023... e9a6075... M > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > :100644 100644 5dd7970... 1ea388f... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > :100644 100644 ad42901... d38d789... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 3466116 > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > a743491 > core/src/main/java/org/apache/sqoop/repository/RepositoryError.java 54e37d9 > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > 19b0023 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 5dd7970 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > ad42901 > > Diff: https://reviews.apache.org/r/26295/diff/ > > > Testing > ------- > > mvn clean test > > > Thanks, > > Abraham Elmahrek > >
