----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52782/#review179086 -----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Line 51 (original), 68 (patched) <https://reviews.apache.org/r/52782/#comment253522> I know you didn't do this, but if we're enhancing stuff, we should import classes separately. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 87 (patched) <https://reviews.apache.org/r/52782/#comment253536> Does it make sense to make this configurable? tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 95 (patched) <https://reviews.apache.org/r/52782/#comment253521> Shouldn't be Class<?> to avoid raw type warning? tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Line 116 (original), 154 (patched) <https://reviews.apache.org/r/52782/#comment253535> 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. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 271 (patched) <https://reviews.apache.org/r/52782/#comment253526> Please make sure you properly document this method. At first glance, it's very unclear what happens here. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 275 (patched) <https://reviews.apache.org/r/52782/#comment253523> Minor: what about defining the size of the underlying array? tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 300 (patched) <https://reviews.apache.org/r/52782/#comment253532> If importEntry is null, this piece of code runs regardless - which isn't a big deal, but we could return earlier. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 349 (patched) <https://reviews.apache.org/r/52782/#comment253524> Class<?> tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 353 (patched) <https://reviews.apache.org/r/52782/#comment253531> Do we have to instantiate the batch handling mechanism just for the sake of a Tx begin/commit? tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 389 (patched) <https://reviews.apache.org/r/52782/#comment253530> Minor: add new line tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 390 (patched) <https://reviews.apache.org/r/52782/#comment253529> Please document the purpose of this class. I would class it BatchTransactionHandler. I feel the word "guard" is slightly misleading. tools/src/main/java/org/apache/oozie/tools/OozieDBImportCLI.java Lines 395 (patched) <https://reviews.apache.org/r/52782/#comment253525> Define the size of the list? tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 21 (patched) <https://reviews.apache.org/r/52782/#comment253538> No stars :) tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Line 26 (original), 29 (patched) <https://reviews.apache.org/r/52782/#comment253537> No stars :) tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 75 (patched) <https://reviews.apache.org/r/52782/#comment253533> Please add a short comment why this is necessary (and what is does) tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 108 (patched) <https://reviews.apache.org/r/52782/#comment253539> Could you move all private methods below the public test methods? tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 116 (patched) <https://reviews.apache.org/r/52782/#comment253534> Minor: line is not properly aligned tools/src/test/java/org/apache/oozie/tools/TestDBLoadDump.java Lines 156 (patched) <https://reviews.apache.org/r/52782/#comment253540> Is this extra method call necessary? - Peter Bacsko On jún. 27, 2017, 10:14 de, András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52782/ > ----------------------------------------------------------- > > (Updated jún. 27, 2017, 10:14 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 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 > >
