> On June 28, 2017, 11:14 a.m., Peter Bacsko wrote: > > tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java > > Line 116 (original), 154 (patched) > > <https://reviews.apache.org/r/52782/diff/3/?file=1763480#file1763480line159> > > > > Wouldn't it be better if this was a public non-static method? That > > would make unit testing easier because we wouldn't have to mess around with > > calling main() method with proper args to perform the DB export/import. The > > main() could still be tested separately. > > > > Altough it would require modifying the export part, too (so that the > > two classes have the same style). > > > > Anyway we can consider this improvement.
Since `TestDBLoadDump` is more of and end-to-end at the moment, I'd leave like it is. > On June 28, 2017, 11:14 a.m., Peter Bacsko wrote: > > tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java > > Lines 353 (patched) > > <https://reviews.apache.org/r/52782/diff/3/?file=1763480#file1763480line374> > > > > Do we have to instantiate the batch handling mechanism just for the > > sake of a Tx begin/commit? I think `BatchTransactionGuard` is pretty lightweight, and it delivers some statistics that may be of good use. It's also a good practice to have appropriate levels of abstraction, and use them. > On June 28, 2017, 11:14 a.m., Peter Bacsko wrote: > > tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java > > Lines 395 (patched) > > <https://reviews.apache.org/r/52782/diff/3/?file=1763480#file1763480line416> > > > > Define the size of the list? To know the exact size we need to iterate through the file anyway, unfortunately. In practice I didn't encounter measurable performance degradation because of using an auto-growing `ArrayList`. > On June 28, 2017, 11:14 a.m., Peter Bacsko wrote: > > tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java > > Lines 156 (patched) > > <https://reviews.apache.org/r/52782/diff/3/?file=1763481#file1763481line159> > > > > Is this extra method call necessary? IMO it makes code more readable. Zero parameters are always better than one ;) - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52782/#review179086 ----------------------------------------------------------- On June 27, 2017, 10:14 a.m., András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52782/ > ----------------------------------------------------------- > > (Updated June 27, 2017, 10:14 a.m.) > > > Review request for oozie, Attila Sasvari, Peter Cseh, Peter Bacsko, and > Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > We get each 1000 rows into a separate JPA `EntityTransaction` to reduce heap > size. Furthermore, of at least one row inside that tx fails, we retry the > whole batch into separate `EntityTransaction`s each. > > Following error handling is implemented: > > 1. check if all necessary tables are present and empty > 2. rows are imported till the end even if there are skipped rows in the > meanwhile > 3. if at least one row is skipped in the meanwhile for some > `ConstraintViolationException`, we delete all rows of all necessary tables. > That enables the user to have the log messages of all the erroneous rows in > one run, and Oozie database is never in an inconsistent state of some rows > present, some not present of an import > > > Diffs > ----- > > tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java 0e14a30 > tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java c43223e > tools/src/test/resources/dumpData/invalid/ooziedb_ac.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_bna.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_bnj.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_ca.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_cj.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_slareg.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_slasum.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_sysinfo.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_wf.json PRE-CREATION > tools/src/test/resources/dumpData/ooziedb_ac.json 22bbdc2 > tools/src/test/resources/dumpData/ooziedb_bna.json e69de29 > tools/src/test/resources/dumpData/ooziedb_bnj.json e69de29 > tools/src/test/resources/dumpData/ooziedb_ca.json 2715b94 > tools/src/test/resources/dumpData/ooziedb_cj.json 979c10e > tools/src/test/resources/dumpData/ooziedb_slareg.json e69de29 > tools/src/test/resources/dumpData/ooziedb_slasum.json e69de29 > tools/src/test/resources/dumpData/ooziedb_sysinfo.json 15de009 > tools/src/test/resources/dumpData/ooziedb_wf.json 05e7e36 > tools/src/test/resources/dumpData/valid/ooziedb_ac.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_bna.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_bnj.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_ca.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_cj.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_slareg.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_slasum.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_sysinfo.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_wf.json PRE-CREATION > > > Diff: https://reviews.apache.org/r/52782/diff/3/ > > > Testing > ------- > > See `TestDBLoadDump` for further reference. > > > Thanks, > > András Piros > >
