[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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. Again, this breaks the API, so we would have to wait for the next major release â 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3836] Add LongHistogram accumulator
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[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
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
