----------------------------------------------------------- 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 > >
