----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21278/#review42740 -----------------------------------------------------------
Ship it! Looks great, thanks for doing this. I have a few comments nothing major. G. core/src/main/java/org/apache/oozie/command/Command.java <https://reviews.apache.org/r/21278/#comment77166> So does it make sense to normalize command metrics? Currently Command and XCommand create different metrics. XCommand has metrics for failure (xexception and exception) and an 'executions' counter. Command does not have these. core/src/main/java/org/apache/oozie/command/XCommand.java <https://reviews.apache.org/r/21278/#comment77204> nit pick: acquireLock does the same, remove it from there as well? core/src/main/java/org/apache/oozie/command/XCommand.java <https://reviews.apache.org/r/21278/#comment77171> does it make sense to add a counter here (it can be the 'exceptions' one or a new one? core/src/main/java/org/apache/oozie/command/XCommand.java <https://reviews.apache.org/r/21278/#comment77168> I think it makes more sense to only time the executions that succeeded. I think that in general you don't want the timer to get data from failed executions that may be very short. core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java <https://reviews.apache.org/r/21278/#comment77174> move it into the try so you won't add failed runs? core/src/main/java/org/apache/oozie/service/InstrumentationService.java <https://reviews.apache.org/r/21278/#comment77176> nit: can't you use (instrumentation == null) for this? core/src/main/java/org/apache/oozie/service/InstrumentationService.java <https://reviews.apache.org/r/21278/#comment77179> nit: what if you are already initialized? Is that okay to call init twice? Also, if you remove the isEnabled you'll need to assign the member at the end to make sure that you only initializes it when init was successful. core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java <https://reviews.apache.org/r/21278/#comment77187> super nit: rename constant to include the unit? core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java <https://reviews.apache.org/r/21278/#comment77190> nit: you don't really need this member. Use the base class member instead? core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java <https://reviews.apache.org/r/21278/#comment77189> same comment about using the instrumentation member itself. core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java <https://reviews.apache.org/r/21278/#comment77196> nit: Why not have the same signature and pass the Instrumentation object? core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java <https://reviews.apache.org/r/21278/#comment77199> nit: I am not sure if it is possible but the actual metric instrumentation instance can change (by calling destroy and init on the metric instrumentation service). So maybe instead of caching a copy of the instrumentation object check the object that is passed in the methods instead? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77216> super nit: this controls not only the logging frequency but also the actual collection frequency, no? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77207> nit: comment or add a readability constant for the no-samples boolean? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77208> can the sliding time window be made configurable? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77212> why not use loading caches for these as well? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77214> nit: Maybe make it into a constant? I believe it is used in a few places. core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77217> >= 0 ? what if something takes less than 1ms? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77209> this can throw if the gauge's name already exist. Why not use a loading cache as you do for counters and timers? This is also true for histograms. core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/21278/#comment77218> super nit: mention that the histogram is biased (decaying)? docs/src/site/twiki/WebServicesAPI.twiki <https://reviews.apache.org/r/21278/#comment77223> mention that this is not an exhaustive list? And as I mentioned off-line, it would be nice if one day oozie can expose an exhaustive list of all possible built-in metrics. - Gilad Wolff On May 13, 2014, 5:40 p.m., Robert Kanter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21278/ > ----------------------------------------------------------- > > (Updated May 13, 2014, 5:40 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1817 > https://issues.apache.org/jira/browse/OOZIE-1817 > > > Repository: oozie-git > > > Description > ------- > > See > https://issues.apache.org/jira/browse/OOZIE-1817?focusedCommentId=13993838&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13993838 > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 808f9b2 > core/pom.xml c935dd7 > core/src/main/java/org/apache/oozie/command/Command.java d3f1011 > core/src/main/java/org/apache/oozie/command/XCommand.java b712ce0 > > core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > e8667c1 > > core/src/main/java/org/apache/oozie/command/coord/CoordActionMaterializeCommand.java > 6962fb2 > > core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java > 57cbb34 > core/src/main/java/org/apache/oozie/service/InstrumentationService.java > 80437b1 > > core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java > PRE-CREATION > core/src/main/java/org/apache/oozie/service/Services.java 5feac7b > core/src/main/java/org/apache/oozie/service/XLogService.java 403a089 > core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java 29d7bd6 > core/src/main/java/org/apache/oozie/servlet/JsonRestServlet.java 5c05acd > core/src/main/java/org/apache/oozie/servlet/V0AdminServlet.java 97bd81c > core/src/main/java/org/apache/oozie/servlet/V1AdminServlet.java a47f737 > core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java 002a367 > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/XLogReporter.java PRE-CREATION > core/src/test/java/org/apache/oozie/service/TestInstrumentationService.java > b5206cf > > core/src/test/java/org/apache/oozie/service/TestMetricsInstrumentationService.java > PRE-CREATION > core/src/test/java/org/apache/oozie/util/TestInstrumentation.java 6040b47 > core/src/test/java/org/apache/oozie/util/TestMetricsInstrumentation.java > PRE-CREATION > docs/src/site/twiki/AG_Install.twiki e343d7e > docs/src/site/twiki/WebServicesAPI.twiki 6730255 > pom.xml b5e0e4e > webapp/src/main/webapp/index.jsp 3c7ffe5 > webapp/src/main/webapp/oozie-console.js 764d888 > > Diff: https://reviews.apache.org/r/21278/diff/ > > > Testing > ------- > > Unit tests, also verified in a cluster > > > Thanks, > > Robert Kanter > >
