sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abraham Elmahrek" <...@cloudera.com>
Subject Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name unique identifier
Date Fri, 03 Oct 2014 18:26:22 GMT


> 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.
> 
> Abraham Elmahrek wrote:
>     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?

I've made the requested changes. Thanks for your feedback guys!


- Abraham


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


On Oct. 3, 2014, 4:06 a.m., Abraham Elmahrek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26295/
> -----------------------------------------------------------
> 
> (Updated Oct. 3, 2014, 4:06 a.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 <abraham@elmahrek.com>
> 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/RepositoryError.java 54e37d9 
>   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/DerbySchemaConstants.java
58eed2d 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
ad42901 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java
4b95687 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
47350ea 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java
8dd1ce2 
> 
> Diff: https://reviews.apache.org/r/26295/diff/
> 
> 
> Testing
> -------
> 
> mvn clean test
> 
> 
> Thanks,
> 
> Abraham Elmahrek
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message