----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21617/#review43331 -----------------------------------------------------------
Looks good - Couple of minor nits. It would be good to add an entry in sqoop-site-template.xml. src/java/org/apache/sqoop/mapreduce/JobBase.java <https://reviews.apache.org/r/21617/#comment77430> Minor nit: Since we are serializing all sqoop options, may be we can set the prefix to be sqoop.option. (or sqoop.opt.) instead of sqoop.arg - Venkat Ranganathan On May 18, 2014, 9:30 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21617/ > ----------------------------------------------------------- > > (Updated May 18, 2014, 9:30 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1321 > https://issues.apache.org/jira/browse/SQOOP-1321 > > > Repository: sqoop-trunk > > > Description > ------- > > I've added code that is serializing the SqoopOptions into job.xml as > suggested on the JIRA. > > Couple of observation: > > * In my tests the job.xml has increased on average by 8% which seems > acceptable > * Password won't be serialized unless explicitly enabled via > sqoop.metastore.client.record.password (the property name is a bit > misleading, but I would prefer not to change to keep backward compatibility) > > > Diffs > ----- > > src/java/org/apache/sqoop/mapreduce/ExportJobBase.java 53de9c9 > src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 8b1493d > src/java/org/apache/sqoop/mapreduce/JobBase.java ddef421 > src/java/org/apache/sqoop/mapreduce/MergeJob.java 5f32127 > > Diff: https://reviews.apache.org/r/21617/diff/ > > > Testing > ------- > > All tests are passing and I've also verified the functionality on real > cluster. > > > Thanks, > > Jarek Cecho > >
