sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 26592: SQOOP-1566: Fix the upgrade logic for SQOOP-1498
Date Mon, 13 Oct 2014 19:40:24 GMT

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


Hi Veena,
I did first pass on the patch and overall it looks good. I do have couple of high level comments
and couple of suggestions below:

1) Having big patches that contains together different functionality is very confusing when
doing a "trip back in history" - for example when debugging some problem or trying to find
out why are certain things written certain way. Can we please split each individual part into
it's own JIRA/RB?

2) I don't have Sqoop 1.99.3 cluster right now, so I did not yet fully tested the ugprade
code. Will do so later.

3) There is a lot of lines with trailing whitespaces - review board is highligting them with
red and also "git apply" command will complain about them. Could you please remove them?

Good work!


common/src/main/java/org/apache/sqoop/model/MConfigurable.java
<https://reviews.apache.org/r/26592/#comment96678>

    This class is missing licence header.



common/src/main/java/org/apache/sqoop/model/MConfigurableType.java
<https://reviews.apache.org/r/26592/#comment96679>

    This class is missing licence header.



core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java
<https://reviews.apache.org/r/26592/#comment96701>

    This rename is not fully applicable here, right? We are indeed expecting mConnector and
not any configurable? E.g. Driver (another configurable) would be incorrect here, right?



core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java
<https://reviews.apache.org/r/26592/#comment96707>

    Similarly here: We are expecting a connector and not general configurable here so the
rename doesn't seem applicable, correct?



core/src/main/java/org/apache/sqoop/driver/Driver.java
<https://reviews.apache.org/r/26592/#comment96709>

    Another example of the same :)



core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java
<https://reviews.apache.org/r/26592/#comment96711>

    Let's not call it "schema" as for example File repository (that don't exist right now)
might not have schema, but just bunch of files and directories. What about just createOrUpgradeRepository()
?



core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java
<https://reviews.apache.org/r/26592/#comment96712>

    Simirly as above I would suggest dropping the "Schema" word.
    
    In addition the "ForUpgrade" don't seem correct as this method is saying whether the repository
is "usable". E.g. it should return true when you can use it directly without any additional
steps. With naming it hasSuitableSchemaForUpgrade it seeems to me that the semantics would
change to "you can't use it directly, you have to do the upgrade first" which is not what
the code then does.



core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java
<https://reviews.apache.org/r/26592/#comment96713>

    What is the perceived difference between "update" and "upgrade" here?



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/26592/#comment96714>

    Similarly as above, I found this semantics change confusing.



core/src/main/resources/driver-config.properties.rej
<https://reviews.apache.org/r/26592/#comment96715>

    This doesn't seem as a valid part of the patch?



execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java
<https://reviews.apache.org/r/26592/#comment96717>

    Since this class is in "mr" package and "mapreduce-execution" module I'm wondering whether
we really need to rename it to "MRConfigurationUtils". I'm probably fine with the rename,
I'm just wondering what we're getting here?



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java
<https://reviews.apache.org/r/26592/#comment96720>

    Do we have a reason to keep it around? Like backward compatibility or something?



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26592/#comment96721>

    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.



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26592/#comment96722>

    Nit: Adding trailing whitespace?



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26592/#comment96723>

    This one seems as left over from debugging. I'm fine with keeping it in, but then we should
make the comment more informative? We might do it in similar way as is line 2410 in the other
log statement, so that we have the same log structure in the method?



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/26592/#comment96724>

    Nit: Trailing whitespace.



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/26592/#comment96725>

    Nite: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/26592/#comment96726>

    Nit: Trailing white spaces



repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java
<https://reviews.apache.org/r/26592/#comment96727>

    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.



repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java
<https://reviews.apache.org/r/26592/#comment96728>

    Seems like a lot of unnecessary changes?



tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java
<https://reviews.apache.org/r/26592/#comment96729>

    Seems like quite a lot of unnecessary changes?


Jarcec

- Jarek Cecho


On Oct. 13, 2014, 4:30 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26592/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2014, 4:30 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> - Mainly fix the upgrade logic for 1498 changes
> - rename the repository upgrader to Configurable upgrader - agreed by Jarcec. 
> 
> - rename configuration utils to MRConfigurationUtils, so it is not be confused with the
sqoop configuration/ configs
> - rename mapreduce to MR ( if this is not acceptable, happy to change it back)
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/job/etl/ExtractorContext.java 3272b56 
>   common/src/main/java/org/apache/sqoop/model/ConfigUtils.java 9e762dc 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.java PRE-CREATION 
>   common/src/main/java/org/apache/sqoop/model/MConfigurableType.java PRE-CREATION 
>   common/src/main/java/org/apache/sqoop/model/MConnector.java 2f42191 
>   common/src/main/java/org/apache/sqoop/model/MDriver.java 685439e 
>   common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 0be4d41 
>   common/src/main/java/org/apache/sqoop/validation/ConfigValidator.java eac789e 
>   common/src/test/java/org/apache/sqoop/model/TestConfigUtils.java d5377f8 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java
87ac2af 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorUpgrader.java
a069b3e 
>   connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConfigUpgrader.java
b17aa21 
>   connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java
606b9fa 
>   connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorUpgrader.java
PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorError.java d544fb1 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 54bdd13 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 5226926 
>   core/src/main/java/org/apache/sqoop/driver/Driver.java f1b45bb 
>   core/src/main/java/org/apache/sqoop/driver/DriverConfigUpgrader.java 847b73d 
>   core/src/main/java/org/apache/sqoop/driver/DriverUpgrader.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/driver/JobManager.java df2a5ab 
>   core/src/main/java/org/apache/sqoop/driver/JobRequest.java 2666320 
>   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/main/resources/driver-config.properties.rej PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/driver/TestDriverConfigUpgrader.java dc4e8c8 
>   core/src/test/java/org/apache/sqoop/driver/TestDriverUpgrader.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/driver/TestJobManager.java 3b475c6 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java e6e4760 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java
47f8478 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/ExecutionError.java PRE-CREATION

>   execution/mapreduce/src/main/java/org/apache/sqoop/job/MapreduceExecutionError.java
1dc12d1 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java 5423b7b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a

>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 03d84d4

>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java
594b5e9 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java
1ebd3e4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopReducer.java a55534a

>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopSplit.java dca4c90 
>   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/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 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java 7109ae5

>   server/src/main/java/org/apache/sqoop/handler/DriverConfigRequestHandler.java aa773a9

>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/SqoopCommand.java cbd34f5 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java PRE-CREATION

>   spi/src/main/java/org/apache/sqoop/connector/spi/RepositoryUpgrader.java 879e428 
>   spi/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java 5315e1f 
>   tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java f89c546 
>   tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java 76ebd3b 
> 
> Diff: https://reviews.apache.org/r/26592/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


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