Hi Julian, Therefore, I ask the dev team: > (1) Do we want a mandatory runtime dependency > io.dropwizard.metrics:metrics-core? >
Ideally we could push it back to optional, as it previously was, but I guess it's a bit more difficult. > (2) Should we revisit OAK-6430 and implement the mechanism via the > facade? Probably extending the HistogramStats interface with a method > "#getPercentile(double)". > Suppose we add #getPercentile(double) in HistogramStats. This means a base implementation should be provided in SimpleStats. But doesn't this defeat the initial purpose for which the metrics library was added/used? I was looking for a simple percentile implementation and that's why we ended up using the one from metrics-core. Using a "dummy" implementation when metrics-core is not available might have a negative impact on the scheduler in SegmentNodeStore. The scheduler uses the percentile information for an optimisation related to some wait time on a semaphore. Inaccurate data could make the optimisation useless and event affect overall performance. But in order to have accurate data, a (decent) percentile implementation is needed. Hence the circle is closed. Regards, Andrei 2017-07-17 12:46 GMT+03:00 Julian Sedding <[email protected]>: > Hi all > > OAK-6430[0] introduced a mandatory dependency to > io.dropwizard.metrics:metrics-core to oak-segment-tar. > > Before this change, the runtime dependency to this metrics library was > optional (and it still is in oak-core). > > Originally, the dependency was introduced in OAK-3654[1] and a facade > was implemented with the following justification: "To avoid having > dependency on Metrics API all over in Oak we can come up with minimal > interfaces which can be used in Oak and then provide an implementation > backed by Metric." > > There was no discussion on the Oak list at the time. However, a > similar discussion happened on the Sling list[2]. Basically, bad past > experiences with breaking changes in the dropwizard metrics API led to > the implementation of a facade in order to limit the potentila impact > of future breaking changes. Of course a facade decouples the code from > the dependency and thus allows plugging in a different implementation > should the need arise. > > Therefore, I ask the dev team: > (1) Do we want a mandatory runtime dependency > io.dropwizard.metrics:metrics-core? > (2) Should we revisit OAK-6430 and implement the mechanism via the > facade? Probably extending the HistogramStats interface with a method > "#getPercentile(double)". > > IMHO we should avoid the mandatory dependency. > > Regards > Julian > > [0] https://issues.apache.org/jira/browse/OAK-6430 > [1] https://issues.apache.org/jira/browse/OAK-3654 > [2] http://markmail.org/thread/47fd5psel2wv2y42 > > > > On Thu, Jul 6, 2017 at 2:54 PM, Andrei Dulceanu > <[email protected]> wrote: > >> The only problem that I see is the fact that it doesn't provide a way to > >> easily access a desired percentile (only mean and 75th, 95th, 98th, 99th > >> and 999th). Currently we are using 50th percentile, i.e. mean, but in > the > >> future that might change. > >> > > > > Please read median instead of mean above. Implementing the change, I > > discovered Histogram#getSnapshot().getValue(double quantile) which is > > exactly what I was looking for. > > > > > >> I will try to make the adjustments and will revisit the percentile > >> implementation once we'll change our use pattern there. > >> > > > > This change is tracked in OAK-6430 [0] and fixed at r1801043. > > > > [0] https://issues.apache.org/jira/browse/OAK-6430 > > > > 2017-07-06 14:55 GMT+03:00 Andrei Dulceanu <[email protected]>: > > > >> Hi Chetan, > >> > >> > >>> Instead of commons-math can we use Metric Histogram (which I also > >>> suggested earlier in the thread). > >> > >> > >> I took another look at the Metric Histogram and I think at the moment it > >> can be used instead of SynchronizedDescriptiveStatistics from > >> commons-math3. The only problem that I see is the fact that it doesn't > >> provide a way to easily access a desired percentile (only mean and 75th, > >> 95th, 98th, 99th and 999th). Currently we are using 50th percentile, > i.e. > >> mean, but in the future that might change. > >> > >> > >>> This would avoid downstream Oak > >>> users to include another dependency as Oak is already using Metrics in > >>> other places. > >>> > >> > >> I will try to make the adjustments and will revisit the percentile > >> implementation once we'll change our use pattern there. > >> > >> Regards, > >> Andrei > >> > >> 2017-07-06 14:38 GMT+03:00 Chetan Mehrotra <[email protected]>: > >> > >>> Instead of commons-math can we use Metric Histogram (which I also > >>> suggested earlier in the thread). This would avoid downstream Oak > >>> users to include another dependency as Oak is already using Metrics in > >>> other places. > >>> > >>> Can we reconsider this decision? > >>> Chetan Mehrotra > >>> > >>> > >>> On Tue, Jul 4, 2017 at 4:45 PM, Julian Sedding <[email protected]> > >>> wrote: > >>> > Maybe it is not necessary to embed *all* of commons-math3. The bnd > >>> > tool (used by maven-bundle-plugin) can intelligently embed classes > >>> > from specified java packages, but only if they are referenced. > >>> > Depending on how well commons-math3 is modularized, that could allow > >>> > for much less embedded classes. Neil Bartlett wrote a good blog post > >>> > about this feature[0]. > >>> > > >>> > Regards > >>> > Julian > >>> > > >>> > [0] http://njbartlett.name/2014/05/26/static-linking.html > >>> > > >>> > > >>> > On Tue, Jul 4, 2017 at 12:20 PM, Andrei Dulceanu > >>> > <[email protected]> wrote: > >>> >> I'll add the dependency. > >>> >> > >>> >> Thanks, > >>> >> Andrei > >>> >> > >>> >> 2017-07-04 13:10 GMT+03:00 Michael Dürig <[email protected]>: > >>> >> > >>> >>> > >>> >>> > >>> >>> On 04.07.17 11:15, Francesco Mari wrote: > >>> >>> > >>> >>>> 2017-07-04 10:52 GMT+02:00 Andrei Dulceanu < > >>> [email protected]>: > >>> >>>> > >>> >>>>> Now my question is this: do we have a simple percentile > >>> implementation in > >>> >>>>> Oak (I didn't find one)? > >>> >>>>> > >>> >>>> > >>> >>>> I'm not aware of a percentile implementation in Oak. > >>> >>>> > >>> >>>> If not, would you recommend writing my own or adapting/extracting > an > >>> >>>>> existing one in a utility class? > >>> >>>>> > >>> >>>> > >>> >>>> In the past we copied and pasted source code from other projects > in > >>> >>>> Oak. As long as the license allows it and proper attribution is > >>> given, > >>> >>>> it shouldn't be a problem. That said, I'm not a big fan of either > >>> >>>> rewriting an implementation from scratch or copying and pasting > >>> source > >>> >>>> code from other projects. Is exposing a percentile really > necessary? > >>> >>>> If yes, how big of a problem is embedding of commons-math3? > >>> >>>> > >>> >>>> > >>> >>> We should avoid copy paste as we might miss important fixes in > later > >>> >>> releases. I only did this once for some code where we needed a fix > >>> that > >>> >>> wasn't yet released. It was a hassle. > >>> >>> I would just add a dependency to commons-math3. Its a library > >>> exposing the > >>> >>> functionality we require, so let's use it. > >>> >>> > >>> >>> Michael > >>> >>> > >>> > >> > >> >
