You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by mbode <gi...@git.apache.org> on 2016/05/06 15:13:22 UTC
[GitHub] flink pull request: [Flink-3836] Add LongHistogram accumulator
GitHub user mbode opened a pull request:
https://github.com/apache/flink/pull/1966
[Flink-3836] Add LongHistogram accumulator
New accumulator `LongHistogram`; the `Histogram` accumulator now throws an `IllegalArgumentException` instead of letting the int overflow.
- [x] 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)
- [x] 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
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mbode/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1966.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 #1966
----
commit f457319481701a1234c9ea7d29da24f857ae4241
Author: Maximilian Bode <ma...@tngtech.com>
Date: 2016-04-27T15:19:16Z
[Flink-3836] Add LongHistogram accumulator
----
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by mbode <gi...@git.apache.org>.
Github user mbode closed the pull request at:
https://github.com/apache/flink/pull/1966
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by mbode <gi...@git.apache.org>.
GitHub user mbode reopened a pull request:
https://github.com/apache/flink/pull/1966
[FLINK-3836] Add LongHistogram accumulator
New accumulator `LongHistogram`; the `Histogram` accumulator now throws an `IllegalArgumentException` instead of letting the int overflow.
- [x] 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)
- [x] 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
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mbode/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1966.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 #1966
----
commit f457319481701a1234c9ea7d29da24f857ae4241
Author: Maximilian Bode <ma...@tngtech.com>
Date: 2016-04-27T15:19:16Z
[Flink-3836] Add LongHistogram accumulator
----
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1966#issuecomment-220275206
I like the addition. Two things, however, that I am not sure about:
1. The `Histogram` uses a `LongHistogram` internally, which results in a lot of wrapping and converting. Each accumulator report (each heartbeat) needs to do the conversion. I think the Histogram should rather hold the proper (int, int) map directly.
2. I am skeptical about failing hard in the Histogram on an integer overflow. These kind of hard failures in utility types are always tricky. A program causing an overflow will result in a non-recoverable failure (it will always overflow again). For streaming programs, that is not a nice property. I would actually rather try to deprecate the int histogram (let it overflow) and encourage to replace it over time completely with the long histogram.
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by mbode <gi...@git.apache.org>.
Github user mbode commented on the pull request:
https://github.com/apache/flink/pull/1966#issuecomment-222155560
Sorry guys, botched the PR :/
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by mbode <gi...@git.apache.org>.
Github user mbode commented on the pull request:
https://github.com/apache/flink/pull/1966#issuecomment-220277425
Hi Stephan, sounds good.
1. I wanted to avoid too much duplication, but I see your point.
2. Ok, throwing a new exception breaks the API. So should I just mark `Histogram` as deprecated? I guess the proper way would be to make Histogram generic, enabling users to instantiate Histogram<Long>. Again, this breaks the API, so we would have to wait for the next major release \u2013 what is the process for cases like that?
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1966#issuecomment-220275432
Please let me know what you think about these suggestions!
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1966#issuecomment-220395032
I think for now, we should add a `@Deprecated` annotation to the `Histogram` and its related method on `RuntimeContext` and mention in the comment that we encourage the `LongHistogram` instead.
On a major release, we should go through all deprecated parts and remove them.
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the pull request:
https://github.com/apache/flink/pull/1966#issuecomment-222161038
I was interested to see what happened here and a simple rebase and force push corrects the problem.
Make sure local master is up-to-date
$ git checkout master
$ git pull apache
Fetch this PR and checkout the branch
$ git fetch github pull/1966/head:pr1966
$ git checkout pr1966
Move the new commits after the last commit on master
$ git rebase master
Push the changes to your repo
$ git push -f pr1966 origin
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by mbode <gi...@git.apache.org>.
Github user mbode closed the pull request at:
https://github.com/apache/flink/pull/1966
---
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.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/1966#discussion_r63847905
--- Diff: flink-streaming-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/testutils/MockRuntimeContext.java ---
@@ -148,6 +149,11 @@ public Histogram getHistogram(String name) {
}
@Override
+ public LongHistogram getLongHistogram(String name) {
--- End diff --
I think we should try and not have utility methods for each accumulator type in the runtime context - it becomes a lot otherwise. The methods for `getHistogram()` etc are also marked as public evolving, because they may possibly be removed in the future.
---
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.
---