New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-4920] Add a Scala Function Gauge #3080

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@heytitle
Contributor

heytitle commented Jan 7, 2017

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General

    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation

    • Documentation has been added for new functionality
    • Old documentation affected by the pull request has been updated
    • JavaDoc for public methods has been added
  • Tests & Build Build Status

    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed
@KurtYoung

Hi @heytitle , thanks for you contribution, i have some comments left in the codes, please feel free to discuss with it if you have some other thoughts.

@KurtYoung

@heytitle Thanks for your work, the changes look good to me, i only have some minor comments.

package org.apache.flink.api.scala.metrics
import org.apache.flink.metrics.Gauge

This comment has been minimized.

@zentol

zentol Jan 11, 2017

Contributor

Can you add a brief description what this class is for?

@zentol

zentol Jan 11, 2017

Contributor

Can you add a brief description what this class is for?

This comment has been minimized.

@heytitle

heytitle Jan 11, 2017

Contributor

IMHO, this class is quite small and obvious what it is doing.

However, if you think it it better to have some explanation, here is my idea.

A ScalaGauge is similar to a Gauge in {@link Metric} but made for Scala API.

What do you think?

@heytitle

heytitle Jan 11, 2017

Contributor

IMHO, this class is quite small and obvious what it is doing.

However, if you think it it better to have some explanation, here is my idea.

A ScalaGauge is similar to a Gauge in {@link Metric} but made for Scala API.

What do you think?

This comment has been minimized.

@zentol

zentol Jan 12, 2017

Contributor

Well, it is certainly easy to see what it does rather quickly. For the description i was thinking more about that it allows the concise definition of a Gauge in Scala, which isn't that obvious for non-scala programmers i suppose.

@zentol

zentol Jan 12, 2017

Contributor

Well, it is certainly easy to see what it does rather quickly. For the description i was thinking more about that it allows the concise definition of a Gauge in Scala, which isn't that obvious for non-scala programmers i suppose.

This comment has been minimized.

@heytitle

heytitle Jan 16, 2017

Contributor

@zentol What do you think about this definition?

ScalaGauge has same functionality as Gauge but it has an advantage
of creating Gauge in more concise way via Scala’s Function0.
@heytitle

heytitle Jan 16, 2017

Contributor

@zentol What do you think about this definition?

ScalaGauge has same functionality as Gauge but it has an advantage
of creating Gauge in more concise way via Scala’s Function0.

This comment has been minimized.

@zentol

zentol Jan 16, 2017

Contributor

I think we can shorten it and borrow something from the jira description:

This class allows the concise definition of a gauge from Scala using lambda notation or function references.

@zentol

zentol Jan 16, 2017

Contributor

I think we can shorten it and borrow something from the jira description:

This class allows the concise definition of a gauge from Scala using lambda notation or function references.

@zentol

This comment has been minimized.

Show comment
Hide comment
@zentol

zentol Jan 17, 2017

Contributor

merging.

Contributor

zentol commented Jan 17, 2017

merging.

zentol added a commit to zentol/flink that referenced this pull request Jan 17, 2017

zentol added a commit to zentol/flink that referenced this pull request Jan 17, 2017

zentol added a commit to zentol/flink that referenced this pull request Jan 19, 2017

zentol added a commit to zentol/flink that referenced this pull request Jan 19, 2017

@asfgit asfgit closed this in 570dbc8 Jan 20, 2017

joseprupi added a commit to joseprupi/flink that referenced this pull request Feb 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment