You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abraham Elmahrek <ab...@cloudera.com> on 2014/10/03 01:22:44 UTC
Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/
-----------------------------------------------------------
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 <ab...@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/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
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Oct. 2, 2014, 11:52 p.m., Veena Basavaraj wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java, line 1254
> > <https://reviews.apache.org/r/26295/diff/1/?file=713258#file713258line1254>
> >
> > these 2 mthods are the same, existLink/Job exvept the sql query, lets please create one method to reuse it.
> >
> > I am not sure the apis should be public at all, why ?
These don't exist any more in this patch.
> On Oct. 2, 2014, 11:52 p.m., Veena Basavaraj wrote:
> > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java, line 214
> > <https://reviews.apache.org/r/26295/diff/1/?file=713255#file713255line214>
> >
> > why public? I dont think we need to expose it.
These don't exist any more in this patch.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55284
-----------------------------------------------------------
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 <ab...@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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55284
-----------------------------------------------------------
core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/26295/#comment95673>
why public? I dont think we need to expose it.
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26295/#comment95672>
these 2 mthods are the same, existLink/Job exvept the sql query, lets please create one method to reuse it.
I am not sure the apis should be public at all, why ?
- Veena Basavaraj
On Oct. 2, 2014, 4: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, 4: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 <ab...@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/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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55452
-----------------------------------------------------------
I'm seeing a test failure when applying last version of the patch:
testUpgradeVersion2ToVersion4(org.apache.sqoop.repository.derby.TestInternals): DERBYREPO_0003:Unable to run specified query - ALTER TABLE SQOOP.SQ_JOB ADD CONSTRAINT SQOOP.FK_SQB_NAME_UNIQUE UNIQUE (SQB_NAME)
- Jarek Cecho
On Oct. 3, 2014, 6:48 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26295/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 6:48 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 <ab...@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 39702ca
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
>
> Diff: https://reviews.apache.org/r/26295/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Gwen Shapira <gs...@cloudera.com>.
> On Oct. 3, 2014, 9:53 p.m., Veena Basavaraj wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java, line 419
> > <https://reviews.apache.org/r/26295/diff/3/?file=713560#file713560line419>
> >
> > nice!
> >
> > I will be adding the same for config table and connfigurable table as well since we will have a separate LINK_TYPE_CONFIG and JOB_TYPE_CONFIG
Wouldn't the configs be managed through FK with Link/Job?
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55403
-----------------------------------------------------------
On Oct. 3, 2014, 6:48 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26295/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 6:48 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 <ab...@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 39702ca
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
>
> Diff: https://reviews.apache.org/r/26295/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Oct. 3, 2014, 9:53 p.m., Veena Basavaraj wrote:
> > core/src/main/java/org/apache/sqoop/repository/RepositoryError.java, line 124
> > <https://reviews.apache.org/r/26295/diff/3/?file=713559#file713559line124>
> >
> > I am becoming more picky about everthing!
> >
> > these should say invalid linf configs/ job configs for connector. Please
I've rebased and most of these were picked up!
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55403
-----------------------------------------------------------
On Oct. 6, 2014, 11:10 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26295/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 11:10 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 <ab...@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 39702ca
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java 1292168
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
>
> Diff: https://reviews.apache.org/r/26295/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55403
-----------------------------------------------------------
Ship it!
love it.!
core/src/main/java/org/apache/sqoop/repository/RepositoryError.java
<https://reviews.apache.org/r/26295/#comment95758>
I am becoming more picky about everthing!
these should say invalid linf configs/ job configs for connector. Please
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26295/#comment95759>
nice!
I will be adding the same for config table and connfigurable table as well since we will have a separate LINK_TYPE_CONFIG and JOB_TYPE_CONFIG
- Veena Basavaraj
On Oct. 3, 2014, 11:48 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, 11:48 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 <ab...@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 39702ca
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
>
> Diff: https://reviews.apache.org/r/26295/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55377
-----------------------------------------------------------
Ship it!
LGTM!
http://www.lgtm.in/i/QApPHEO0q
- Gwen Shapira
On Oct. 3, 2014, 6:48 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26295/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 6:48 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 <ab...@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 39702ca
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
>
> Diff: https://reviews.apache.org/r/26295/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/
-----------------------------------------------------------
(Updated Oct. 6, 2014, 11:10 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 <ab...@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 (updated)
-----
core/src/main/java/org/apache/sqoop/repository/RepositoryError.java 54e37d9
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 39702ca
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java 1292168
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
Diff: https://reviews.apache.org/r/26295/diff/
Testing
-------
mvn clean test
Thanks,
Abraham Elmahrek
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/
-----------------------------------------------------------
(Updated Oct. 3, 2014, 6:48 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 <ab...@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 (updated)
-----
core/src/main/java/org/apache/sqoop/repository/RepositoryError.java 54e37d9
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 39702ca
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java fc3ec18
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 951d9b4
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java bf72626
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java 595b1c8
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java 38e632a
Diff: https://reviews.apache.org/r/26295/diff/
Testing
-------
mvn clean test
Thanks,
Abraham Elmahrek
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
New review with Gwen's and Veena's feedback.
NOTE: Upgrades will fail if there are non-null names that are the same for 2 different jobs or 2 different links.
Bugs: SQOOP-1477
https://issues.apache.org/jira/browse/SQOOP-1477
Repository: sqoop-SQOOP-1367
Description
-------
commit 450db6146922bc197bb7c3eda375319788d610ba
Author: Abraham Elmahrek <ab...@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 (updated)
-----
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
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Oct. 2, 2014, 4: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
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.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55277
-----------------------------------------------------------
On Oct. 2, 2014, 4: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, 4: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 <ab...@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/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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Oct. 2, 2014, 4: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.
+ 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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55277
-----------------------------------------------------------
On Oct. 2, 2014, 4: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, 4: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 <ab...@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/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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> 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 <ab...@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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> 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 :)
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.
- 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 <ab...@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/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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Gwen Shapira <gs...@cloudera.com>.
> 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.
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.
- Gwen
-----------------------------------------------------------
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 <ab...@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/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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> 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 <ab...@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/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
>
>
Re: Review Request 26295: SQOOP-1477: Sqoop2: Make link and job name
unique identifier
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26295/#review55277
-----------------------------------------------------------
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26295/#comment95666>
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 :)
- Gwen Shapira
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 <ab...@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/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
>
>