You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2018/02/16 08:34:27 UTC

[GitHub] flink pull request #5501: [FLINK-6053][metrics] Add new Number-/StringGauge ...

GitHub user zentol opened a pull request:

    https://github.com/apache/flink/pull/5501

    [FLINK-6053][metrics] Add new Number-/StringGauge metric types

    ## What is the purpose of the change
    
    This PR deprecates the `Gauge` metric type and introduces 2 new metric types: `StringGauge` and `NumberGauge`. The generic nature of `Gauges` is a big headache as it encourages exposing arbitrary objects, which not a single reporter actually supports. Furthermore, making the type distinction also proved to be difficult, as gauges may return null which fails `instanceof` checks.
    
    This change is for the most part backwards compatible:
    * `StringGauges` are wrapped such that they are registered as both `Gauge<String>` and `StringGauge`
    * `NumberGauges` are wrapped such that they are registered as both `Gauge<Number>` and `NumberGauge`
    * `Gauges<?>` are wrapper such that they are registered as both `Gauge<?>` and `StringGauge`.
    
    Thus, legacy reporters still work with the new metric types, and old metrics still work with new reporters, provided the reporter supports strings. We _could_ improve the compatibility for old metrics by checking the return type of the gauge for whether it is a number and register a `NumberGauge` instead.
    
    A new overloaded method `void register` was added to `MetricGroup`. We could not overload `gauge()` as this would forbid lambdas from being used without explicit class declaration.
    The signature of `register` is different than other registration methods. Neither do they return the given metric nor do they have generic parameters. This is done to support lambdas, as they have inherent problems with generic methods (ambiguity). This change has no effect on our code-base as we nowhere made use of this feature (which i think is still neat).
    
    The first commit introduces the new types, deprecates Gauges and sets up the compatibility layer.
    The second commit updates all reporters to work against the new metric types.
    The third commit replaces all existing usages of `Gauges` to instead use the new metric types.
    
    ## Verifying this change
    
    Existing tests were updated.
    
    ## Does this pull request potentially affect one of the following parts:
    
      - Dependencies (does it add or upgrade a dependency): (no)
      - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes)
      - The serializers: (no)
      - The runtime per-record code paths (performance sensitive): (no)
      - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
      - The S3 file system connector: (no)
    
    ## Documentation
    
    The documentation was not updated yet.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/zentol/flink 6053

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/flink/pull/5501.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #5501
    
----
commit 7bc13f9f8dcb814fa88b746c1839511a639b234e
Author: zentol <ch...@...>
Date:   2018-02-07T10:11:56Z

    [FLINK-6053][metrics] Add new Number-/StringGauge metric types

commit 85a48c02fbd58edc822949cf9f9177c9c36661cc
Author: zentol <ch...@...>
Date:   2018-02-07T10:34:50Z

    update reporters

commit 2046d3ee79adeb0528169686c3fddf980232614d
Author: zentol <ch...@...>
Date:   2018-02-14T14:26:04Z

    replace all usages of now deprecated Gauge

----


---