----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62459/#review186940 -----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 77 (patched) <https://reviews.apache.org/r/62459/#comment263839> I think this should go to stdout instead tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 113 (patched) <https://reviews.apache.org/r/62459/#comment263846> I see writeString("\n") at least a dozen of times (actually 18). I strongly suggest adding a method to the writer and call it writeNewLine(). tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleCollectorDriver.java Lines 41 (patched) <https://reviews.apache.org/r/62459/#comment263842> We might as we call this DATE_FORMAT. Also, if it's referenced from other classes (and I see it is), just make it public. tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleCollectorDriver.java Lines 47 (patched) <https://reviews.apache.org/r/62459/#comment263843> I know that this is returned by the parser, but can we switch to List<String> here if it's not a big deal? It just feels safer. tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleCompressor.java Lines 31 (patched) <https://reviews.apache.org/r/62459/#comment263847> Nit: I know that zip() is expressive in itself, but still, the name compress() sounds better to me. tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleEntryWriter.java Lines 48 (patched) <https://reviews.apache.org/r/62459/#comment263840> This is usually thrown by the JVM when a static initializer block fails. Could we change this to something else? An ordinary RuntimeException looks good enough to me. tools/src/main/java/org/apache/oozie/tools/diag/MetricsCollector.java Lines 46 (patched) <https://reviews.apache.org/r/62459/#comment263844> Nit: space :D tools/src/main/java/org/apache/oozie/tools/diag/MetricsCollector.java Lines 52 (patched) <https://reviews.apache.org/r/62459/#comment263845> Do we need to call flush() here? tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java Lines 56 (patched) <https://reviews.apache.org/r/62459/#comment263848> Could this whole example metrics be stored in a file and not inline in the code? tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java Lines 122 (patched) <https://reviews.apache.org/r/62459/#comment263852> Simpler: import static java.nio.file.Files.readAllBytes; import static java.nio.file.Paths.get; String s = new String(readAllBytes(get("test.txt")))); This part should be extracted somewhere to avoid repeated coding to load a file tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java Lines 127 (patched) <https://reviews.apache.org/r/62459/#comment263855> The string "UTF-8" is repeated a couple of times. Make it const or use StandardCharsets.UTF_8. tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java Lines 134 (patched) <https://reviews.apache.org/r/62459/#comment263849> Same here, I would store this in an external file tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java Lines 215 (patched) <https://reviews.apache.org/r/62459/#comment263853> Simplify file loading tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java Lines 231 (patched) <https://reviews.apache.org/r/62459/#comment263850> Handle this catch properly tools/src/test/java/org/apache/oozie/tools/diag/TestServerInfoCollector.java Lines 120 (patched) <https://reviews.apache.org/r/62459/#comment263854> Simplify file loading - Peter Bacsko On okt. 2, 2017, 9:39 de, Attila Sasvari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62459/ > ----------------------------------------------------------- > > (Updated okt. 2, 2017, 9:39 de) > > > Review request for oozie. > > > Repository: oozie-git > > > Description > ------- > > A diagnostic tool that collects a bunch of job and other information from > Oozie in a zip file. > > > Diffs > ----- > > docs/src/site/twiki/DG_CommandLineTool.twiki > d4047671876dcc3279a2ec379bc1d003f5e6f1aa > pom.xml 0b94484da1c97618e9168cea0ebbfff7f70f723c > tools/pom.xml 7306a14e7b237977be00f8fe28e34573540fd508 > tools/src/main/bin/oozie-diag-bundle-collector.sh PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/ArgParser.java PRE-CREATION > > tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleCollectorDriver.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleCompressor.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/DiagBundleEntryWriter.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/DiagOozieClient.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/MetricsCollector.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/ServerInfoCollector.java > PRE-CREATION > tools/src/test/java/org/apache/oozie/tools/diag/TestAppInfoCollector.java > PRE-CREATION > tools/src/test/java/org/apache/oozie/tools/diag/TestArgParser.java > PRE-CREATION > tools/src/test/java/org/apache/oozie/tools/diag/TestMetricsCollector.java > PRE-CREATION > > tools/src/test/java/org/apache/oozie/tools/diag/TestServerInfoCollector.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/62459/diff/9/ > > > Testing > ------- > > - new unit tests: TestOozieDiagBundleCollector > - started Oozie with a pseudo hadoop cluster, submitted a couple workflows, > and executed the following commands: > -- ``bin/oozie-diag-bundle-collector.sh`` (usage info printed), > -- ``bin/oozie-diag-bundle-collector.sh -numworkflows 2000 -oozie > http://localhost:11000/oozie -output /tmp``, > -- ``bin/oozie-diag-bundle-collector.sh -jobs > 0000001-170918144116149-oozie-asas-W -oozie http://localhost:11000/oozie > -output .`` (verified zip the tool generated). > > > Thanks, > > Attila Sasvari > >
