----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21898/#review43910 -----------------------------------------------------------
Good work Gwen! I do have couple of high level notes: 1) Can you please add documentation? https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Tools.rst 2) Absence of tests is currently expected as we don't have testing infrastructure for tools. tools/pom.xml <https://reviews.apache.org/r/21898/#comment78199> Let's put all the dependencies into root pom.xml file, including the versions. We do have <dependencyManagement> section for that. tools/src/main/java/org/apache/sqoop/tools/tool/BuiltinTools.java <https://reviews.apache.org/r/21898/#comment78200> The "datadump" might suggest that Sqoop will do some data transfer. What about using word "repo" or "repository" instead? Something like "repositorydump", "repodump" or "repositoryexport", "repoexport"? tools/src/main/java/org/apache/sqoop/tools/tool/DataDumpTool.java <https://reviews.apache.org/r/21898/#comment78201> This file is missing Apache license header. tools/src/main/java/org/apache/sqoop/tools/tool/DataDumpTool.java <https://reviews.apache.org/r/21898/#comment78202> Please don't use "*" imports, always iterate all used symbols. tools/src/main/java/org/apache/sqoop/tools/tool/DataDumpTool.java <https://reviews.apache.org/r/21898/#comment78203> We are recommending to shut down server for all tools: https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Tools.rst tools/src/main/java/org/apache/sqoop/tools/tool/DataDumpTool.java <https://reviews.apache.org/r/21898/#comment78204> Nit: "include-sensitive" might be a bit better name? Yeah I kind of don't like the word "dump" :-( tools/src/main/java/org/apache/sqoop/tools/tool/DataDumpTool.java <https://reviews.apache.org/r/21898/#comment78205> I think that the idea is that user should be able to use the dump to load data back to repository (e.g. for backup purpose or during migration from one repository to another one). Something like mysqldump & mysqlimport. Hence two high level notes: * I would assume that the extra messages "dumping ..." will cause issues when parsing the text. * We do need to output some information about the connectors though. The connection will contain connector ID without specifying what exact connector has been there - this might be a trouble as different connectors might have different IDs on different repositories. We don't necessary have to print out entire connector info, but at least the associated unique identification. - Jarek Cecho On May 25, 2014, 6:56 p.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21898/ > ----------------------------------------------------------- > > (Updated May 25, 2014, 6:56 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 > ----- > > 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/DataDumpTool.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 > >
