----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21898/#review48179 -----------------------------------------------------------
Good work Gwen! Couple of high level notes: 1) Please always put the patch on JIRA as well, we do have pre-commit build that will test your changes. Also we can't commit your changes unless they are attached to JIRA. 2) The loader tool will load entire repository dump into memory which seems fine for now, but we might need to think about file format that would enable us process the dump in streaming fashion in the future. tools/pom.xml <https://reviews.apache.org/r/21898/#comment84503> Super nit: The formatting seems to be off here. tools/src/main/java/org/apache/sqoop/tools/tool/BuiltinTools.java <https://reviews.apache.org/r/21898/#comment84504> Super nit: Please put space between command and the "RepositoryXTool.class..." tools/src/main/java/org/apache/sqoop/tools/tool/JSONConstants.java <https://reviews.apache.org/r/21898/#comment84511> I can't help by my OCD is complaining about the location of the file - it's in generic package, but only two tools are using it. What about creating sub-package "repository" and put repository related tools there? tools/src/main/java/org/apache/sqoop/tools/tool/JSONConstants.java <https://reviews.apache.org/r/21898/#comment84510> I don't think that interface is the correct type here, what about final class? tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84512> Nit: Unused import. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84513> Nit: Unused import. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84514> Nit: Unused import. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84505> Shouldn't the default value be NULL? tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84506> Shouldn't we on parse error return false to indicate failure? tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84507> Super nit: Please put spaces after the commas... tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java <https://reviews.apache.org/r/21898/#comment84515> Wouldn't be cleaner to just use ConnectorManager.getConnector API to get the connector name? https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java#L135 tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84508> Not sure if this is indeed needed import. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84516> Nit: Please use "//" for one line comments. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84509> What about creating another parent tool that will initialize entire Sqoop infrastructure? Something like the ConfiguredTool - we should be able to use the same tool for dump and load. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84517> Nit: Please do convert this to the standard Java doc comment. tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84518> I'm thinking if this check is indeed necessary. User might want to load older backup to a different Sqoop version right? tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84520> Similar update code is already in the code base, so I'm wondering if it would make sense to abstract and reuse it rather then have similar code on two places? https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/repository/Repository.java#L386 tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java <https://reviews.apache.org/r/21898/#comment84519> Seems like good use case to use ConnectorManager.getConnector() API? https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java#L140 Jarcec - Jarek Cecho On July 18, 2014, 4:11 p.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21898/ > ----------------------------------------------------------- > > (Updated July 18, 2014, 4:11 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Added tool for dumping user-generated data - connections, jobs and > submissions. There's an option to dump sensitive data (i.e. passwords) as > well. > > > Diffs > ----- > > docs/src/site/sphinx/Tools.rst ad72cd1 > pom.xml 1e2f005 > tools/pom.xml 31eda1c > tools/src/main/java/org/apache/sqoop/tools/tool/BuiltinTools.java b24cb35 > tools/src/main/java/org/apache/sqoop/tools/tool/JSONConstants.java > PRE-CREATION > tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java > PRE-CREATION > tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/21898/diff/ > > > Testing > ------- > > Manual testing. Dumping repository with and without sensitive data. > Validating resulting JSON. > > > Thanks, > > Gwen Shapira > >
