----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52782/#review179560 -----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 413 (patched) <https://reviews.apache.org/r/52782/#comment254342> Could you rephrese the first sentence pls? It sounds a bit cryptic. Possible sentence I can think of: "Persists entities in batches, that is, the actual commit will be done when the number of persistable entities reach the batch limit." Just an example. Also explain why we need to commit in batches (IIRC it was because of OOME, right?) tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 467 (patched) <https://reviews.apache.org/r/52782/#comment254340> Please check if extra text is necessary after newEntity (JDK8 javadoc is very strict) tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 484 (patched) <https://reviews.apache.org/r/52782/#comment254341> Same here tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 488 (patched) <https://reviews.apache.org/r/52782/#comment254334> Is it possible that currentTransaction() is already open at this point? If I'm not mistaken, calling begin() again results in an exception being thrown. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 505 (patched) <https://reviews.apache.org/r/52782/#comment254338> Java8 javadoc checker will whine about the missing text after @return. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 513 (patched) <https://reviews.apache.org/r/52782/#comment254339> Same here tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 234 (patched) <https://reviews.apache.org/r/52782/#comment254335> Another Class<?> :) tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 241 (patched) <https://reviews.apache.org/r/52782/#comment254337> If the class passed here can only be WorkflowActionBean, does it make sense to have this argument at all? tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 264 (patched) <https://reviews.apache.org/r/52782/#comment254336> Class<?> - Peter Bacsko On júl. 4, 2017, 10:09 de, András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52782/ > ----------------------------------------------------------- > > (Updated júl. 4, 2017, 10:09 de) > > > 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 > 0e14a30693a76b8b2bdc2f7ceaf3f045d69f4155 > tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java > c43223ef05aa702be49565ba2626314628e63749 > tools/src/test/resources/dumpData/invalid/ooziedb_ac.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_sysinfo.json PRE-CREATION > tools/src/test/resources/dumpData/invalid/ooziedb_wf.json PRE-CREATION > tools/src/test/resources/dumpData/ooziedb_ac.json > tools/src/test/resources/dumpData/ooziedb_bna.json > tools/src/test/resources/dumpData/ooziedb_bnj.json > tools/src/test/resources/dumpData/ooziedb_ca.json > tools/src/test/resources/dumpData/ooziedb_cj.json > tools/src/test/resources/dumpData/ooziedb_slareg.json > tools/src/test/resources/dumpData/ooziedb_slasum.json > tools/src/test/resources/dumpData/ooziedb_sysinfo.json > tools/src/test/resources/dumpData/ooziedb_wf.json > 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_slareg.json PRE-CREATION > tools/src/test/resources/dumpData/valid/ooziedb_slasum.json PRE-CREATION > > > Diff: https://reviews.apache.org/r/52782/diff/4/ > > > Testing > ------- > > See `TestDBLoadDump` for further reference. > > > Thanks, > > András Piros > >
