You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Robert Kanter <rk...@cloudera.com> on 2014/05/09 21:14:43 UTC
Review Request 21278: OOZIE-1817: Oozie timers are not biased
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/
-----------------------------------------------------------
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
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java, line 37
> > <https://reviews.apache.org/r/21278/diff/3/?file=580650#file580650line37>
> >
> > nit: you don't really need this member. Use the base class member instead?
>
> Robert Kanter wrote:
> MetricsInstrumentation has a destroy() method that we have to call to stop the Metrics stuff from logging. Instrumentation doesn't have this, so I need it to be a MetricsInstrumentation object and not simply an Instrumentation object (and I'd rather not deal with casting).
I ended up doing this because I made some other changes and didn't need the destroy() anymore :)
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review42740
-----------------------------------------------------------
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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 105
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line105>
> >
> > can the sliding time window be made configurable?
>
> Robert Kanter wrote:
> I need to investigate this some more. I'm wondering if this is the right type of Reservoir to be using here.
>
> According to http://metrics.codahale.com/manual/core/#exponentially-decaying-reservoirs SlidingTimeWindowReservoir can be memory intensive and they recommend ExponentiallyDecayingReservoir instead (which does roughly the last 5min with some fancy algorithm). That's also what I'm using for the samplers/histograms.
>
> There's also the SlidingWindowReservoir, which stores the last N measurements (SlidingTimeWindowReservoir does the last N seconds/minutes).
>
> Thoughts?
I'm going to use the ExponentiallyDecayingReservoir because of the memory usage. We don't want someone configuring the SlidingTimeWindowReservoir to eat up a ton of memory.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review42740
-----------------------------------------------------------
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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > Looks great, thanks for doing this. I have a few comments nothing major.
> >
> > G.
Thanks. I've answered most of the issues below and made code changes or explained why not. There's a few things I'll have to look into more and I need to do some testing for some of the changes. I'll try to post an updated patch tomorrow.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/command/Command.java, line 195
> > <https://reviews.apache.org/r/21278/diff/3/?file=580644#file580644line195>
> >
> > 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.
I checked and we currently only have one Command (CoordActionMaterializeCommand) that is using Command and not XCommand. Plus, it's only being used during a dryrun operation so it's not frequently used. I think it would be better to just convert CoordActionMaterializeCommand to an CoordActionMaterializeXCommand and remove Command completely; I've opened OOZIE-1846 for that.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/command/XCommand.java, line 332
> > <https://reviews.apache.org/r/21278/diff/3/?file=580645#file580645line332>
> >
> > 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.
The "call" timer includes other stuff than just the main part of the command, including loading state, verifying a precondition, etc. I think we should keep the "call" timer as-is so it includes these other things. For example, if a command fails its precondition a lot, the timer should show that (failing the precondition throws an exception).
However, the "execute" timer only includes the main part of the command and only gets updated if there's no exceptions. So I think this timer covers essentially what you're looking for here.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/service/InstrumentationService.java, line 51
> > <https://reviews.apache.org/r/21278/diff/3/?file=580649#file580649line51>
> >
> > 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.
You're not allowed to init() twice without first doing destroy(). I imagine that many of the Services would have problems if you tried to do that. We don't have anything enforcing this really (and it's actually caused problems in tests before), but for now we just assume you don't.
That's is a good point about assigning the member at the end.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java, line 34
> > <https://reviews.apache.org/r/21278/diff/3/?file=580650#file580650line34>
> >
> > super nit: rename constant to include the unit?
I know Hadoop usually does that, but Oozie doesn't. We just mention it in the description in oozie-default/oozie-site and/or the documentation. I had forgotten to add this to oozie-default.xml, so thanks for reminding me though.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java, line 37
> > <https://reviews.apache.org/r/21278/diff/3/?file=580650#file580650line37>
> >
> > nit: you don't really need this member. Use the base class member instead?
MetricsInstrumentation has a destroy() method that we have to call to stop the Metrics stuff from logging. Instrumentation doesn't have this, so I need it to be a MetricsInstrumentation object and not simply an Instrumentation object (and I'd rather not deal with casting).
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java, line 397
> > <https://reviews.apache.org/r/21278/diff/3/?file=580653#file580653line397>
> >
> > nit: Why not have the same signature and pass the Instrumentation object?
We generate the json from the Instrumentation and MetricsInstrumentation differently, so this can't be done without a lot of refactoring. I think it's simpler to just have two methods.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java, line 48
> > <https://reviews.apache.org/r/21278/diff/3/?file=580657#file580657line48>
> >
> > 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?
We actually cache the regular Instrumentation in other places too. While the Services can technically be destroyed and then re-init again, in practice, you can't do that without restarting Oozie, in which case V2AdminServlet gets recreated and it's not a problem. I'd imagine that restarting the Services without restarting Oozie would lead to other similar problems too.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 70
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line70>
> >
> > super nit: this controls not only the logging frequency but also the actual collection frequency, no?
Nope, just the logging frequency. It's only used by the XLogReporter. The collection frequency, at least for the samplers/histograms is whatever is passed to addSampler(...).
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 105
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line105>
> >
> > can the sliding time window be made configurable?
I need to investigate this some more. I'm wondering if this is the right type of Reservoir to be using here.
According to http://metrics.codahale.com/manual/core/#exponentially-decaying-reservoirs SlidingTimeWindowReservoir can be memory intensive and they recommend ExponentiallyDecayingReservoir instead (which does roughly the last 5min with some fancy algorithm). That's also what I'm using for the samplers/histograms.
There's also the SlidingWindowReservoir, which stores the last N measurements (SlidingTimeWindowReservoir does the last N seconds/minutes).
Thoughts?
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 111
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line111>
> >
> > why not use loading caches for these as well?
Can't do that because we have to implement a Gauge object and return the value from the Variable object, which we don't have in the CacheLoader's load(String key) method. We have to create the Gauge and then put it into the Map.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 147
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line147>
> >
> > >= 0 ? what if something takes less than 1ms?
The reason I added that was that if you add a cron that was never actually run, without the if statement, you'll add an extra 0 value to the timer. I'll look into what to do about this.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 173
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line173>
> >
> > 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.
I address why not using a loading cache in a previous comment. In general, Gauges and Histograms are registered only once by classes that implement Instrumentable so that shouldn't be a problem; the other metrics are added on the fly. However, just in case, I've added a check that if the key already exists to remove it before re-adding it.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > docs/src/site/twiki/WebServicesAPI.twiki, line 279
> > <https://reviews.apache.org/r/21278/diff/3/?file=580665#file580665line279>
> >
> > 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.
I added something about this to the docs (it's also true for instrumentation).
I agree that it would be nice. Unfortunately, that's kind of tricky because timers and counters are registered on the fly and many are dynamically generated by subclasses. This would likely require an offline tool that would look through the code and try to figure it out. The other metrics/instrumentation types are all registered on Oozie startup though.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review42740
-----------------------------------------------------------
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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 15, 2014, 8:55 p.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 147
> > <https://reviews.apache.org/r/21278/diff/3/?file=580658#file580658line147>
> >
> > >= 0 ? what if something takes less than 1ms?
>
> Robert Kanter wrote:
> The reason I added that was that if you add a cron that was never actually run, without the if statement, you'll add an extra 0 value to the timer. I'll look into what to do about this.
I've removed that check. The proper usage pattern of the cron object is to run them before adding them.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review42740
-----------------------------------------------------------
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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Gilad Wolff <no...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 30, 2014, 9 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java, line 53
> > <https://reviews.apache.org/r/21278/diff/6/?file=588573#file588573line53>
> >
> > where is the disabling of instrumentation endpoint taking place?
In V2AdminServlet, if (metricsInstrumentation == null) sendMetricsResponse(...) will response when a SC_SERVICE_UNAVAILABLE; sentInstrumentationReponse(...) will do the opposite.
> On May 30, 2014, 9 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 165
> > <https://reviews.apache.org/r/21278/diff/6/?file=588574#file588574line165>
> >
> > why use lock when guages is a concurrenthashmap?
Besides adding to the gauges hashmap, addVariable(...) also adds and/or removes the gauge to/from the metricsRegistry so that entire set of operations should be thread safe
> On May 30, 2014, 9 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 221
> > <https://reviews.apache.org/r/21278/diff/6/?file=588574#file588574line221>
> >
> > same as above
Same as before, but for histograms instead of gauges.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review44419
-----------------------------------------------------------
On May 22, 2014, 11:27 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 11:27 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Mona Chitnis <mo...@yahoo.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review44419
-----------------------------------------------------------
some more comments in addition to the couple before
core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java
<https://reviews.apache.org/r/21278/#comment78796>
where is the disabling of instrumentation endpoint taking place?
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment78797>
nitpick- typo retrieve
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment78798>
why use lock when guages is a concurrenthashmap?
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment78799>
same as above
- Mona Chitnis
On May 22, 2014, 11:27 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 11:27 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/
-----------------------------------------------------------
(Updated June 16, 2014, 8:09 p.m.)
Review request for oozie.
Changes
-------
Final patch with the minor changes described here: https://issues.apache.org/jira/browse/OOZIE-1817?focusedCommentId=14032865&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14032865
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 (updated)
-----
client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 97fd465
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 19f867e
core/src/main/java/org/apache/oozie/command/coord/CoordActionMaterializeCommand.java 6962fb2
core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java 515e247
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 0739cae
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/XLog.java 21e00c0
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 ce84e54
docs/src/site/twiki/WebServicesAPI.twiki 5769924
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
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On June 16, 2014, 6:35 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java, line 115
> > <https://reviews.apache.org/r/21278/diff/6/?file=588562#file588562line115>
> >
> > so no counter for inputcheck command?
One of the parent classes, XCommand, already takes care of a few different crons (e.g. ".execute"), so this cron is unnecessary. Plus, it wasn't ever even being reported because instrumentation.addCron(...) was never being called for it anyway.
I'll double-check that crons are being using properly and then upload the latest patch (there were the minor things you pointed out before such as a variable name having a typo, and I got rid of the log4j meters) to JIRA so Jenkins can run it.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review45783
-----------------------------------------------------------
On May 22, 2014, 11:27 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 11:27 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Mona Chitnis <mo...@yahoo.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review45783
-----------------------------------------------------------
patch looks good. Please ensure that the cron.start() and cron.stop() are present in all the required commands as I didnt check that myself. After that +1
core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java
<https://reviews.apache.org/r/21278/#comment80763>
so no counter for inputcheck command?
- Mona Chitnis
On May 22, 2014, 11:27 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 11:27 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Mona Chitnis <mo...@yahoo.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review44075
-----------------------------------------------------------
Will look into Codahale a bit more in detail and review again.
core/src/main/java/org/apache/oozie/service/MetricsInstrumentationService.java
<https://reviews.apache.org/r/21278/#comment78405>
is this logging interval going to be different than InstrumentationService.logging.interval? if not, can use just the parent attribute
core/src/main/java/org/apache/oozie/servlet/JsonRestServlet.java
<https://reviews.apache.org/r/21278/#comment78409>
typo JSON
- Mona Chitnis
On May 22, 2014, 11:27 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 11:27 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/
-----------------------------------------------------------
(Updated May 22, 2014, 11:27 p.m.)
Review request for oozie.
Changes
-------
https://issues.apache.org/jira/browse/OOZIE-1817?focusedCommentId=14006604&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14006604
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 (updated)
-----
client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 97fd465
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/XLog.java 21e00c0
core/src/main/resources/oozie-default.xml 9788daf
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 5769924
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
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 21, 2014, 4:24 a.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 137
> > <https://reviews.apache.org/r/21278/diff/4-5/?file=583904#file583904line137>
> >
> > I believe that Cache gives you the semantics you want and you don't need the additional lock. That is, get-and-load-if-not-found is atomic from the perspective of the get caller. This is true for the counters as well.
The cache itself is thread-safe, but Timer and Counter aren't necessarily so.
> On May 21, 2014, 4:24 a.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 169
> > <https://reviews.apache.org/r/21278/diff/4-5/?file=583904#file583904line169>
> >
> > Sorry I missed this but maybe add a log statement if you encounter a gauge with the same name. This shouldn't happen.
The old Instrumentation actually throws a RuntimeException if this happens, so it really shouldn't happen. But it's probably useful to add a DEBUG level log statement.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review43585
-----------------------------------------------------------
On May 19, 2014, 6:46 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 19, 2014, 6:46 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Gilad Wolff <no...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review43585
-----------------------------------------------------------
Ship it!
nit-picks.
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment77848>
I believe that Cache gives you the semantics you want and you don't need the additional lock. That is, get-and-load-if-not-found is atomic from the perspective of the get caller. This is true for the counters as well.
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment77849>
Sorry I missed this but maybe add a log statement if you encounter a gauge with the same name. This shouldn't happen.
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment77850>
and a log statement here as well.
- Gilad Wolff
On May 19, 2014, 6:46 p.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 19, 2014, 6:46 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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/
-----------------------------------------------------------
(Updated May 19, 2014, 6:46 p.m.)
Review request for oozie.
Changes
-------
https://issues.apache.org/jira/browse/OOZIE-1817?focusedCommentId=14002155&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14002155
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 (updated)
-----
client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 97fd465
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/XLog.java 21e00c0
core/src/main/resources/oozie-default.xml 9788daf
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 5769924
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
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
> On May 19, 2014, 4:10 a.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java, line 53
> > <https://reviews.apache.org/r/21278/diff/4/?file=583903#file583903line53>
> >
> > super nit: this seems like a very long line.
We use max 132 characters per line (IMO, this is much easier to read than Hadoop's 80), so this is fine.
> On May 19, 2014, 4:10 a.m., Gilad Wolff wrote:
> > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java, line 150
> > <https://reviews.apache.org/r/21278/diff/4/?file=583904#file583904line150>
> >
> > this is not thread-safe. make the function synchronized? Also, change the function comment as the variable may exists now, it will just be replaced with the latest.
Good point; I wasn't thinking about thread safety. I looked back at Instrumentation and they use a lock for each type of metric; so I'm going to do it that way.
> On May 19, 2014, 4:10 a.m., Gilad Wolff wrote:
> > core/src/test/java/org/apache/oozie/service/TestMetricsInstrumentationService.java, line 30
> > <https://reviews.apache.org/r/21278/diff/4/?file=583908#file583908line30>
> >
> > super nit: long line?
< 132
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review43333
-----------------------------------------------------------
On May 17, 2014, 12:09 a.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 17, 2014, 12:09 a.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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Gilad Wolff <no...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/#review43333
-----------------------------------------------------------
Ship it!
Thanks for the changes. A couple of comments and nit-picks.
G.
core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java
<https://reviews.apache.org/r/21278/#comment77431>
super nit: this seems like a very long line.
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment77433>
this is not thread-safe. make the function synchronized? Also, change the function comment as the variable may exists now, it will just be replaced with the latest.
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java
<https://reviews.apache.org/r/21278/#comment77432>
this is not thread-safe as well.
core/src/test/java/org/apache/oozie/service/TestMetricsInstrumentationService.java
<https://reviews.apache.org/r/21278/#comment77434>
super nit: long line?
- Gilad Wolff
On May 17, 2014, 12:09 a.m., Robert Kanter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21278/
> -----------------------------------------------------------
>
> (Updated May 17, 2014, 12:09 a.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 97fd465
> 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/XLog.java 21e00c0
> core/src/main/resources/oozie-default.xml 9788daf
> 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 5769924
> 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
>
>
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/
-----------------------------------------------------------
(Updated May 17, 2014, 12:09 a.m.)
Review request for oozie.
Changes
-------
See https://issues.apache.org/jira/browse/OOZIE-1817?focusedCommentId=14000539&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14000539
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 (updated)
-----
client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 97fd465
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/XLog.java 21e00c0
core/src/main/resources/oozie-default.xml 9788daf
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 5769924
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
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
New patch fixes relevant test failure (TestMetricsInstrumentationService).
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 (updated)
-----
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
Re: Review Request 21278: OOZIE-1817: Oozie timers are not biased
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21278/
-----------------------------------------------------------
(Updated May 12, 2014, 8:53 p.m.)
Review request for oozie.
Changes
-------
Fixed a test problem
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 (updated)
-----
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