----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10134/#review18401 -----------------------------------------------------------
Hi Abe, thank you very much for working on this patch. I do have few small nits: src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/10134/#comment38598> Please do not change the classes in com.cloudera packages. They are provided only for backward compatibility with old extensions. Any new extension should move to org.apache package. src/java/org/apache/sqoop/SqoopOptions.java <https://reviews.apache.org/r/10134/#comment38599> Nit: s/iimport/import/ src/java/org/apache/sqoop/tool/ImportAllTablesTool.java <https://reviews.apache.org/r/10134/#comment38600> What about simplifying this to something like: excludes.addAll(options.getAllTablesExclude().split(",")); src/java/org/apache/sqoop/tool/ImportAllTablesTool.java <https://reviews.apache.org/r/10134/#comment38601> Can we make an "else" clause printing out INFO message about skipping the table? I know that it seems redundant as the user must have specified it on command line, but I think that it can help us when helping users on mailing lists. Jarcec - Jarek Cecho On March 26, 2013, 6:36 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10134/ > ----------------------------------------------------------- > > (Updated March 26, 2013, 6:36 a.m.) > > > Review request for Sqoop, Jarek Cecho and Kathleen Ting. > > > Description > ------- > > commit 2898f99a2a0009c3a84a8d327f1be7b0c6a4fc7e > Author: Abraham Elmahrek <[email protected]> > Date: Mon Mar 25 23:25:13 2013 -0700 > > SQOOP-885 Allow excluding some tables from import-all-tables tools > > Added argument "--exclude-tables" which accepts a comma separated > list of tables to exclude from the import process. > Added a test case to validate table import exclusion. > Updated documentation. > > :100644 100644 5d340b9... 6b639f5... M > src/docs/man/sqoop-import-all-tables.txt > :100644 100644 9f9bc88... 8c3a4f5... M > src/docs/user/import-all-tables.txt > :100644 100644 a5f72f7... 3ca4a87... M > src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java > :100644 100644 08bab1e... 64e7ada... M > src/java/org/apache/sqoop/SqoopOptions.java > :100644 100644 9874bae... 59308de... M > src/java/org/apache/sqoop/tool/BaseSqoopTool.java > :100644 100644 158a3f1... 76c8daf... M > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java > :100644 100644 133bc8f... ce87eac... M > src/test/com/cloudera/sqoop/TestAllTables.java > > > This addresses bug SQOOP-885. > https://issues.apache.org/jira/browse/SQOOP-885 > > > Diffs > ----- > > src/docs/man/sqoop-import-all-tables.txt > 5d340b9967488c2d1d6e72d2f4d69aa8f4906bda > src/docs/user/import-all-tables.txt > 9f9bc885bf6464093604a90303b28839297586ce > src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java > a5f72f7a6280cbe53914a8628bce032120e7d2f4 > src/java/org/apache/sqoop/SqoopOptions.java > 08bab1e4d39824b25bc055bb8a7b96fde18cad81 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java > 9874bae773d5de260a8859d9f1b0349b56a89b12 > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java > 158a3f18823ae4ea7511d1b71a75a23b4458809d > src/test/com/cloudera/sqoop/TestAllTables.java > 133bc8f69be1f7e7f3cf0a788779de4156cc27bd > > Diff: https://reviews.apache.org/r/10134/diff/ > > > Testing > ------- > > Added test case. Executed import all test cases. > > > Thanks, > > Abraham Elmahrek > >
