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 >>> >>> >>> >> >>
