----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62459/#review186754 -----------------------------------------------------------
docs/src/site/twiki/DG_CommandLineTool.twiki Lines 1948 (patched) <https://reviews.apache.org/r/62459/#comment263555> It would be good to add something about the children aspect of the tool (e.g. that if you get a coordinator, it will also get some number of child workflows) tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 97-98 (patched) <https://reviews.apache.org/r/62459/#comment263557> Something to consider for all output of the tool, not just here: we're outputting most of the info in a human-readable format. Should we think about using a machine-readable format? Or maybe having the option for one? Or doing both? The idea being that someone would then be able to write their own tool that could analyze stuff. We already have some code somewhere that converts a WorkflowJob into JSON, so it shouldn't be a lot of work to add this either. That might also be a good idea from a compatibility perspective - i.e. what's the compatibility story on this out? If there's a new field, what do we do? tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 99-119 (patched) <https://reviews.apache.org/r/62459/#comment263556> I'm not sure that's necessary here - we're not concatinating strings, we're writing to a Writer. tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 151 (patched) <https://reviews.apache.org/r/62459/#comment263558> Should we skip control types? That's probably fine for the start action, but what about a decision action? That has some useful info. Maybe we should still print these out, but with fewer fields or something (e.g. no need for a console url). tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 163 (patched) <https://reviews.apache.org/r/62459/#comment263604> I think the JHS may also be required, in the cases where the RM has forgotten about the job. And what about HDFS? That's required too. I'm thinking we might be best off not doing these checks. It's too complicated (CM spent a lot of effort on this) and we can't check for everything (e.g. what if log aggregation is turned off?). Besides, we're already handling exceptions below when trying to get the logs - if the RM, JHS, HDFS, etc isn't working, the call will fail anyway. tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 185 (patched) <https://reviews.apache.org/r/62459/#comment263560> Please create a followup JIRA to change this in the future to use OOZIE-2983 ("Stream the Launcher AM Logs") once it's done. This will also be nice in that we can get rid of the RM up check. tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 191 (patched) <https://reviews.apache.org/r/62459/#comment263559> Is there not a cleaner way to do this than using a CLI like this? tools/src/main/java/org/apache/oozie/tools/diag/AppInfoCollector.java Lines 221 (patched) <https://reviews.apache.org/r/62459/#comment263561> This won't work right if using RM HA... I'd recommend using a ````YarnClient```` and passing it the ````hadoopConfig```` so it can figure out the RM address for you. There must be a benign simple ````YarnClient```` command you can run to verify connectivity. tools/src/main/java/org/apache/oozie/tools/diag/BundleCollectorDriver.java Lines 37 (patched) <https://reviews.apache.org/r/62459/#comment263605> I'm not sure I like the name "BundleXYZ" for these classes. It's ambiguous with a Bundle Job. Perhaps "DiagBundleXYZ" instead? tools/src/main/java/org/apache/oozie/tools/diag/BundleCompressor.java Lines 32 (patched) <https://reviews.apache.org/r/62459/#comment263606> Can use the Java 7 try-with-resources. tools/src/main/java/org/apache/oozie/tools/diag/Client.java Lines 32 (patched) <https://reviews.apache.org/r/62459/#comment263607> I think you have a typo here: "... so we get that for free" tools/src/main/java/org/apache/oozie/tools/diag/Client.java Lines 38 (patched) <https://reviews.apache.org/r/62459/#comment263609> I would add a comment here explaining why we're doing this. (i.e. that Oozie has a jsp page with a thread dump, and the Oozie client normally doesn't have a way of getting it, so we're doing that here; reusing all the fancy HTTP handling code in AuthOozieClient) tools/src/main/java/org/apache/oozie/tools/diag/MetricsCollector.java Lines 47-49 (patched) <https://reviews.apache.org/r/62459/#comment263608> Strange lining here tools/src/main/java/org/apache/oozie/tools/diag/ServerInfoCollector.java Lines 45-47 (patched) <https://reviews.apache.org/r/62459/#comment263610> Strange lining here tools/src/test/java/org/apache/oozie/tools/diag/TestAppInfoCollector.java Lines 48 (patched) <https://reviews.apache.org/r/62459/#comment263611> I know the ````@Test```` indicates that this is a test, but it's still convention to name all tests starting with "test". So, "testGetLastNWorkflows()". Same for the others. - Robert Kanter On Sept. 27, 2017, 2:12 p.m., Attila Sasvari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62459/ > ----------------------------------------------------------- > > (Updated Sept. 27, 2017, 2:12 p.m.) > > > 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 efccc346932514ada578a3462eb3c3cfe519a323 > 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/BundleCollectorDriver.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/BundleCompressor.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/BundleEntryWriter.java > PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/diag/Client.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/7/ > > > 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 > >
