> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java, line 141
> > <https://reviews.apache.org/r/26657/diff/1/?file=719676#file719676line141>
> >
> > Simirly as above I would suggest dropping the "Schema" word.
> >
> > In addition the "ForUpgrade" don't seem accurate as this method
> > implementation is not checking whether the repository is ready for upgrade.
> > It's checking that the repo is ready to be used. E.g. if this methods
> > return true then Sqoop will continue and start repository and won't run
> > upgrade as one would expect by the changed name.
I am not sure what the history of this method was. But as far the current
implmentation is, it checks this , i,e if the version in the repository
database is what the code states ( harcoded constant)
@Override
public boolean hasSuitableSchemaForUpgrade(Connection conn) {
// TODO(jarcec): Verify that all structures are present (e.g. something
like corruption validation)
// NOTE: At this point is is just checking if the repo version matches the
version
// in the upgraded code
return detectRepositoryVersion(conn) ==
DerbyRepoConstants.LATEST_DERBY_REPOSITORY_VERSION;
}
if(!provider.getRepository().hasSuitableSchemaForUpgrade()) {
throw new SqoopException(RepositoryError.REPO_0002);
}
this is how this method used today, and it throws an exception if the
schema is not ready for upgrade. So What does "Suitable" really mean here.
Might be good to think about it as well
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/Repository.java, lines 63-66
> > <https://reviews.apache.org/r/26657/diff/1/?file=719678#file719678line63>
> >
> > If we want to be that specific then we should also state that this
> > method will be called during upgrade tool:
> >
> >
> > https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Upgrade.rst#upgrading-server-using-upgrade-tool
Sure we should add it, I did not know it existed:)
BTW, this link to upgrade tool is broken on git
https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Upgrade.rst#upgrading-server-using-upgrade-tool
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java,
> > lines 24-28
> > <https://reviews.apache.org/r/26657/diff/1/?file=719681#file719681line24>
> >
> > Do we have a reason to keep it around? Like backward compatibility or
> > something?
yes, the test code the way it is written requires it.
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java,
> > lines 333-337
> > <https://reviews.apache.org/r/26657/diff/1/?file=719683#file719683line333>
> >
> > I'm wondering what are your thoughts about having multiple repositories
> > at this level? I mean this is a derbo repy implementation, so this code
> > will always deal with one single derby repo, correct? If sqoop will ever
> > need more then one repository, then we will have multiple instances of this
> > code pointing to different databases.
sure, but we have more than one type of version stored in repository already.
driver verson. So the comment should probably reflect that, I will update it.
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java,
> > lines 20-55
> > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line20>
> >
> > Is this change intentional?
yes it is, it is always hard to do merges with every single import added.
I would like to understand if this is performance concern/ nitpick or is there
some standards we follow for the import order. I did not see it documented
anywhere, would be nice to have a pointer to the code style expected.
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java,
> > lines 96-97
> > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line96>
> >
> > We should keep this constant here as well - again it's intentionally
> > duplicated here, so that the developer is intentionally changing it on two
> > places to realize need to do the upgrade.
why would we have duplication like this in the code? Test code usually uses the
same constants. This is asking for trouble to have duplicate code in test,
which I have already seen is true in many places in the sqoop code, refactoring
breaks it
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java,
> > lines 205-206
> > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line205>
> >
> > The reason why we have the code duplicated here, is that if somebody
> > will unintentionally change the code in DerbyRepositoryHandler, then all
> > tests will start breaking and he will realize that the change has been
> > invalid as it requires an upgrade path. Hence unless we will somehow
> > accomplish this goal differently, I would prefer to keep this particular
> > code duplication.
There are other ways to handle such things, than duplicating code, I am happy
to create a ticket for this and fix it the right way, instead of debating. I
really dont like how copy paste is considered a solution for things.
> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java,
> > lines 20-56
> > <https://reviews.apache.org/r/26657/diff/1/?file=719689#file719689line20>
> >
> > Nit: Seems as unnecessary changes?
same as above, is there a import order imposed so I can use that as a standard
in the IDE. If we have more developers contributing to the code base, projects
use a style guide as a standard to avoid debating on such things:). Time well
spent on real issues.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26657/#review56443
-----------------------------------------------------------
On Oct. 13, 2014, 1:03 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26657/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 1:03 p.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> fix the upgrade logic for 1498 changes.
> While fixing the upgrade logic, added comments on how the upgrade logic works.
> -renamed test methods to reflect what they are upgrading and what they are
> infact testing.
> -renamed the internals to exactly mean "schema" upgrades, since we only
> allow/ do scheme upgrades the repository
> -Also updated the names on the repository api to clearly state the difference
> between upgrade and update operations on connector / driver.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 3ade247
> core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
> 97de893
> core/src/main/java/org/apache/sqoop/repository/Repository.java 95c7a4d
> core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java
> c2f8505
> core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java
> e6e4760
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java
> 74e41df
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
> 0f0f7c4
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
> 10a7b1a
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java
> cf6e657
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
> 56ea147
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java
> 9316687
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java
> fc95222
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java
> d597bd8
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java
> 260c2a9
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java
> 0eb9df4
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
> 01a05b2
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java
> bbfe5bb
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java
> PRE-CREATION
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java
> 8402d8c
>
> Diff: https://reviews.apache.org/r/26657/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
