flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zentol <...@git.apache.org>
Subject [GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Date Thu, 16 Jun 2016 11:11:19 GMT
Github user zentol commented on the issue:

    https://github.com/apache/flink/pull/2112
  
    -1. I haven't looked very deeply at the actual code but let me share some thoughts:
    
    I was under the impression that the core of this issue was to implement at least 1 Histogram
that does not rely on another library. It _should_ contain at least one to guarantee that
the compatibility layer works properly.
    
    The original PR for the metric system already contained wrapped DropWIzard Histograms,
which were specifically taken out as they did not meet our performance needs. Why we now essentially
revert this decision now 2 weeks later is beyond me. I also hope you did not spend too much
time writing this as a lot of similar code already existed.
    
    As such I really don't see the point of the DropWizardWrapper. IMO the only thing we need
is the HistogramWrapper class, which may require a more distinctive name btw. . We should
not expose a DW -> Flink compatibility layer, next up people want to use DW Timers, Gauges
etc. as well. This will simply create more work for us.
    
    The original PR also contained the flink-metric-reporters module as flink-metrics. Now
we revert this again as well. Just...wth.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message