----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36512/#review123964 -----------------------------------------------------------
Overall looks good. Thanks for all of the effort you put into this. I haven't had a chance to apply the patch and play around with it (I'll try to do that soon), but here's some comments in the meantime. client/src/main/java/org/apache/oozie/client/OozieClient.java (line 2049) <https://reviews.apache.org/r/36512/#comment186312> I think we should also add a CLI command for the health check data. It can call the JSON page and parse it to print out to the console nicely (similarly to what I did in OOZIE-2174). client/src/main/java/org/apache/oozie/client/OozieClient.java (line 2154) <https://reviews.apache.org/r/36512/#comment186288> This isn't a very informative comment... client/src/main/java/org/apache/oozie/client/OozieClient.java (line 2197) <https://reviews.apache.org/r/36512/#comment186287> response client/src/main/java/org/apache/oozie/client/OozieClient.java (line 2205) <https://reviews.apache.org/r/36512/#comment186289> getResponse client/src/main/java/org/apache/oozie/client/OozieClient.java (line 2206) <https://reviews.apache.org/r/36512/#comment186290> Use StringBuilder instead of StringBuffer core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java (line 731) <https://reviews.apache.org/r/36512/#comment186292> I don't see this being used anywhere. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java (line 743) <https://reviews.apache.org/r/36512/#comment186293> I don't see this being used anywhere. core/src/main/java/org/apache/oozie/healthcheck/DataBaseHealthCheck.java (line 34) <https://reviews.apache.org/r/36512/#comment186294> Would it be better to check something with the OOZIE_SYS table? I'm no database expert, but given that we don't normally touch the OOZIE_SYS table, but we would touch the BUNDLE_JOBS table for someone with lots of Bundles, wouldn't it be less of a performance impact (i.e. less/no lock contention) if we use OOZIE_SYS? In any case, I'm not a big fan of the E0604 and E0605 handling. We should pick a query (or create a new one) that always works. core/src/main/java/org/apache/oozie/healthcheck/HdfsHealthCheck.java (line 52) <https://reviews.apache.org/r/36512/#comment186299> Perhaps this should be debug level? core/src/main/java/org/apache/oozie/healthcheck/HdfsHealthCheck.java (lines 66 - 68) <https://reviews.apache.org/r/36512/#comment186300> If you have multiple paths, these values will be whatever the last path looked at is. For example, if pathA failed but pathB succeeded, these will have OK status. We probably want to have it be ERROR if one or more paths failed. core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutor.java (line 60) <https://reviews.apache.org/r/36512/#comment186301> Extra set of brackets here core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutor.java (line 100) <https://reviews.apache.org/r/36512/#comment186302> Would it make sense to also report these in the instrumentation/metrics? core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutorHTML.java (line 28) <https://reviews.apache.org/r/36512/#comment186303> Alternatively, we could add a new tab or subtab to the Web Console that calls the JSON one. core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutorJson.java (line 52) <https://reviews.apache.org/r/36512/#comment186305> I'm not sure we should do this. The HTTP code should be for the page itself, which is OK, even if a component failed a health check. The consumer of the JSON should determine the health based on the JSON content. core/src/main/java/org/apache/oozie/healthcheck/HealthCheckStatus.java (line 28) <https://reviews.apache.org/r/36512/#comment186306> Why Throwable and not Exception? core/src/main/resources/oozie-default.xml (line 2347) <https://reviews.apache.org/r/36512/#comment186307> Add a description core/src/test/java/org/apache/oozie/healthcheck/TestZKHealthCheck.java (line 29) <https://reviews.apache.org/r/36512/#comment186308> It should be possible to do a negative test here by shutting down the embedded ZooKeeper docs/src/site/twiki/AG_Install.twiki (line 1010) <https://reviews.apache.org/r/36512/#comment186311> We also need docs for the new validate command in the CLI docs page and the REST API docs page. And getting the JSON for the health check should also be in the REST API docs page. And if we add the CLI too (an earlier comment), then the CLI docs need to incldue this as well. docs/src/site/twiki/AG_Install.twiki (line 1023) <https://reviews.apache.org/r/36512/#comment186309> Can we make this a bullet list? It will be easier to read and add more in the future. I think we should also include the "org.apache.oozie.healthcheck." part, or I'm sure they'll be some user who makes a mistake here. docs/src/site/twiki/AG_Install.twiki (line 1025) <https://reviews.apache.org/r/36512/#comment186310> I think we should add a note to the Oozie HA section about the ZKHealthCheck with a link to this section. - Robert Kanter On Feb. 9, 2016, 5:28 p.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36512/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2016, 5:28 p.m.) > > > Review request for oozie. > > > Repository: oozie-git > > > Description > ------- > > Oozie can be configured to check the health of its components. > http://localhost:11000/oozie/v2/admin/health will report the health of > configured components and time taken to compute health check (this can be > used to find component slowness). > Health check component can be configured with below properties. > <verbatim> > <property> > <name>oozie.healthCheck.components</name> > <value>org.apache.oozie.healthcheck.DataBaseHealthCheck</value> > <description> > </description> > </property> > </verbatim> > Support components are DataBaseHealthCheck, HdfsHealthCheck, > SharelibHealthCheck, SharelibMetaFileHealthCheck and ZKHealthCheck. > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/cli/OozieCLI.java > 65291ff1303cb0e8419e2930521d26c0b3cd7142 > client/src/main/java/org/apache/oozie/client/OozieClient.java > 67a62c622a9ef86afb82b492735d146faba427de > client/src/main/java/org/apache/oozie/client/rest/JsonTags.java > b3b148acb8e0b71b6233937b793453009a4faf2a > client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 3c6931915a1afb3ead4b12e1b9ea3c02835c0b5d > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 4fc0c52396165ea9e8add98368e360e3ec989b77 > core/src/main/java/org/apache/oozie/healthcheck/DataBaseHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/healthcheck/HdfsHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutor.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutorHTML.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/healthcheck/HealthCheckExecutorJson.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/healthcheck/HealthCheckStatus.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/healthcheck/OozieHealthCheckComponent.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/healthcheck/SharelibHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/main/java/org/apache/oozie/healthcheck/SharelibMetaFileHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/healthcheck/ZKHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/service/ShareLibService.java > 81a9f2d5da781ff79b9bcb1cb5cca02575d57b49 > core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java > 64d3f1f40c5c7b3746ad410cf43f80925424d094 > core/src/main/java/org/apache/oozie/servlet/JsonRestServlet.java > 059d3cf6dc251b49940af29d82cbdd817043a176 > core/src/main/java/org/apache/oozie/servlet/V0AdminServlet.java > fd573d51b77d86c77e944b4d49e1f77f30eb6a26 > core/src/main/java/org/apache/oozie/servlet/V1AdminServlet.java > 965a19a2f14e4ba10e5977bf042a2d3c2e47d525 > core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java > 7eadbe781e58f5bb674930288416ac4e7f0c3ae0 > core/src/main/resources/oozie-default.xml > 3ff7320df003a0ee05f767ffcf3d9a2b0b448d07 > core/src/test/java/org/apache/oozie/client/TestOozieCLI.java > 54bfc16131ed20218ef56e028a69a5507432bd1a > > core/src/test/java/org/apache/oozie/healthcheck/TestDataBaseHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > > core/src/test/java/org/apache/oozie/healthcheck/TestSharelibMetaFileHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/java/org/apache/oozie/healthcheck/TestZKHealthCheck.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/java/org/apache/oozie/service/ShareLibServiceTestCases.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/test/java/org/apache/oozie/service/TestShareLibService.java > 842b2d53ed1f996f694c8960384e8455a9923372 > docs/src/site/twiki/AG_Install.twiki > 66c00199a3ef457cd495ec8b9717823b820d958a > > Diff: https://reviews.apache.org/r/36512/diff/ > > > Testing > ------- > > Added enough test cases and checked manually. > > > Thanks, > > Purshotam Shah > >
