You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Qian Xu <sx...@googlemail.com> on 2014/11/26 03:49:38 UTC
Review Request 28463: Sqoop2: Repository upgrade issue will prevent
server startup
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-1812
https://issues.apache.org/jira/browse/SQOOP-1812
Repository: sqoop-sqoop2
Description
-------
Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
Diffs
-----
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
Diff: https://reviews.apache.org/r/28463/diff/
Testing
-------
Added a test case to guard the scenario
Thanks,
Qian Xu
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63100
-----------------------------------------------------------
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java
<https://reviews.apache.org/r/28463/#comment105268>
sure this test makes sense now that we will still be ok with double call to upgrade. thanks for the update Qian.
please rename the method, that we are doing double upgrade and please add a comment, it really gets harder to understand these othwerise later.
- Veena Basavaraj
On Nov. 25, 2014, 10:11 p.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2014, 10:11 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/
-----------------------------------------------------------
(Updated Dec. 4, 2014, 10:37 a.m.)
Review request for Sqoop.
Changes
-------
if repository version is already up-to-date, no schema change check will be executed.
Bugs: SQOOP-1812
https://issues.apache.org/jira/browse/SQOOP-1812
Repository: sqoop-sqoop2
Description
-------
Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
Diffs (updated)
-----
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
Diff: https://reviews.apache.org/r/28463/diff/
Testing
-------
Added a test case to guard the scenario
Thanks,
Qian Xu
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 26, 2014, 1:07 a.m., Abraham Elmahrek wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java, line 46
> > <https://reviews.apache.org/r/28463/diff/4/?file=776638#file776638line46>
> >
> > The comments above need to be updated, but this looks good.
>
> Veena Basavaraj wrote:
> on 1.99.4, this test case fails
>
> If there is a double upgrade initiated, it should not throw an exception on 1.99.4.
>
> Also changing to a new version is relevant when new upgrade code is added in 1.99.5, does not seem right to up a version number otherwise.
> We have not even done that. And it is not trivial change
>
> Second, if it tied to a release that number needs to be then tied to the release number and not a random incrementing sequence like we have.
I have better understanding of this issue now.
One of the greatest mysteries of sqoop2
The following code was added when the release was NOT yet to be finalized and it was in sqoop2, so it actually assumed that the current release version is 4.
if (repositoryVersion <= 4) {
runQuery(QUERY_UPGRADE_TABLE_SQ_CONFIG_ADD_UNIQUE_CONSTRAINT_NAME_TYPE_AND_CONFIGURABLE_ID,
conn);
runQuery(QUERY_UPGRADE_TABLE_SQ_INPUT_ADD_UNIQUE_CONSTRAINT_NAME_TYPE_AND_CONFIG_ID, conn);
}
Then it was committed to Sqoop2, but not cherry picked for 1.99.4. So potentially this code belongs to the next release i.e 1.99.5.
It is amazing how we have designed the repository code that depends on the cherry picking:)
Mt 2 cents if anyone cares to listen.
1. Now in between the releases if we are adding new upgrade code, it is so not obvious that it changed to 5.
2. These repo version should be tied to some build release numbers/ tags that update automatically when a branch is cut. It cannot be a hardcoded value in the code.
We should fix this.
Second just changing the version number wont fix it, we have to update tests and add test cases for verion2 - verion 5, version 4- version 5.
We need update everywhere in the code that is hardcoded to handle only version 2 or version 4 now.!
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63102
-----------------------------------------------------------
On Nov. 26, 2014, 1:05 a.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 1:05 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java d869cb7
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 26, 2014, 1:07 a.m., Abraham Elmahrek wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java, line 46
> > <https://reviews.apache.org/r/28463/diff/4/?file=776638#file776638line46>
> >
> > The comments above need to be updated, but this looks good.
on 1.99.4, this test case fails
If there is a double upgrade initiated, it should not throw an exception on 1.99.4.
Also changing to a new version is relevant when new upgrade code is added in 1.99.5, does not seem right to up a version number otherwise.
We have not even done that. And it is not trivial change
Second, if it tied to a release that number needs to be then tied to the release number and not a random incrementing sequence like we have.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63102
-----------------------------------------------------------
On Nov. 26, 2014, 1:05 a.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 1:05 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java d869cb7
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63102
-----------------------------------------------------------
Ship it!
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java
<https://reviews.apache.org/r/28463/#comment105274>
The comments above need to be updated, but this looks good.
- Abraham Elmahrek
On Nov. 26, 2014, 9:05 a.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 9:05 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java d869cb7
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/
-----------------------------------------------------------
(Updated Nov. 26, 2014, 5:05 p.m.)
Review request for Sqoop.
Changes
-------
Regard Abe's comments, the most recent repository version should be 5
Bugs: SQOOP-1812
https://issues.apache.org/jira/browse/SQOOP-1812
Repository: sqoop-sqoop2
Description
-------
Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
Diffs (updated)
-----
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java d869cb7
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
Diff: https://reviews.apache.org/r/28463/diff/
Testing
-------
Added a test case to guard the scenario
Thanks,
Qian Xu
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/
-----------------------------------------------------------
(Updated Nov. 26, 2014, 2:35 p.m.)
Review request for Sqoop.
Changes
-------
get a better name for the test case
Bugs: SQOOP-1812
https://issues.apache.org/jira/browse/SQOOP-1812
Repository: sqoop-sqoop2
Description
-------
Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
Diffs (updated)
-----
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
Diff: https://reviews.apache.org/r/28463/diff/
Testing
-------
Added a test case to guard the scenario
Thanks,
Qian Xu
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/
-----------------------------------------------------------
(Updated Nov. 26, 2014, 2:11 p.m.)
Review request for Sqoop.
Changes
-------
polished test case
Bugs: SQOOP-1812
https://issues.apache.org/jira/browse/SQOOP-1812
Repository: sqoop-sqoop2
Description
-------
Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
Diffs (updated)
-----
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
Diff: https://reviews.apache.org/r/28463/diff/
Testing
-------
Added a test case to guard the scenario
Thanks,
Qian Xu
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63096
-----------------------------------------------------------
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/28463/#comment105261>
nm, seems like we dont have a check to not enter this code path if the version is already the current version, !
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java
<https://reviews.apache.org/r/28463/#comment105260>
so on fresh install there is no repo, hence a version = 0, upgrade is done and repo version is set to 4 .
Next we restart and repo version is 4, it is hard to capture this in a test case. So I am not clear what we will be asserting.
Second, calling both of these, is not going to help
super.createOrUpgradeSchemaForLatestVersion();
58
handler.createOrUpgradeRepository(getDerbyDatabaseConnection());
- Veena Basavaraj
On Nov. 25, 2014, 6:49 p.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2014, 6:49 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63094
-----------------------------------------------------------
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/28463/#comment105258>
is this new issue? I am wondering how we missed his earlier?
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java
<https://reviews.apache.org/r/28463/#comment105257>
I might some more help understanding the test case.
- Veena Basavaraj
On Nov. 25, 2014, 6:49 p.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2014, 6:49 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>
Re: Review Request 28463: Sqoop2: Repository upgrade issue will
prevent server startup
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28463/#review63098
-----------------------------------------------------------
Ship it!
I could reproduce too..Great catch.!
lets remove unit test and commit the actual fix ! a follow up for an integration test that can simulate this across sqoop servers starts, its hard to automate this still.
- Veena Basavaraj
On Nov. 25, 2014, 6:49 p.m., Qian Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28463/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2014, 6:49 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1812
> https://issues.apache.org/jira/browse/SQOOP-1812
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Unexpected behaviour: Sqoop2 server tries to add an existing constraint repeatedly.
>
>
> Diffs
> -----
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 907978f
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java 928c34a
>
> Diff: https://reviews.apache.org/r/28463/diff/
>
>
> Testing
> -------
>
> Added a test case to guard the scenario
>
>
> Thanks,
>
> Qian Xu
>
>