You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by HuangWHWHW <gi...@git.apache.org> on 2015/09/21 15:27:00 UTC
[GitHub] flink pull request: [FLINK-2720][storm-compatibility]Add Storm-Cou...
GitHub user HuangWHWHW opened a pull request:
https://github.com/apache/flink/pull/1157
[FLINK-2720][storm-compatibility]Add Storm-CountMetric for storm metrics
I added the Storm-CountMetric for the first step about storm metrics.
1.Do a wrapper `FlinkCountMetric` for the CountMetric.
2.There is a real metric LongCounter in FlinkCountMetric class.
3.Push the RuntimeContext in `FlinkTopologyContext` for registering the metric.
4.Add a simple ut for the `FlinkCountMetric` .
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HuangWHWHW/flink FLINK-2720
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1157.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 #1157
----
commit 13c696beef318ef947b0e616fa9e29258bcbbf86
Author: HuangWHWHW <40...@qq.com>
Date: 2015-09-21T13:22:02Z
[FLINK-2720][storm-compatibility]Add Storm-CountMetric for storm metrics
----
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on the issue:
https://github.com/apache/flink/pull/1157
I added https://github.com/apache/flink/pull/3615, welcome for comments.
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-141985771
This looks promising. However, how can `CountMetric` be used (without any changes to the Spout/Bolt code within Flink? Right now it seems, that the user has to replace `CountMetric` with `FlinkCountMetric` in `open()/prepare()` method. However, the user code should be unchanged and the counter should be collected by Flink under the hood, and translated into a Flink accumulator transparently to the user. I found a nice example on the usage of metrics in Storm: https://www.endgame.com/blog/storm-metrics-how Maybe you try to run this code just changing `TopologyBuilder` to `FlinkTopologyBuilder` and omitting the metric consumer for now.
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by HuangWHWHW <gi...@git.apache.org>.
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-142176942
@mjsax
Yes, I got the same example as you :D
And I try to add an example to show how to use this FlinkCountMetric by following the example.
---
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 #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMe...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on a diff in the pull request:
https://github.com/apache/flink/pull/1157#discussion_r104311065
--- Diff: flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkTopologyContext.java ---
@@ -120,7 +129,19 @@ public ReducedMetric registerMetric(final String name, final IReducer combiner,
@SuppressWarnings("unchecked")
@Override
public IMetric registerMetric(final String name, final IMetric metric, final int timeBucketSizeInSecs) {
- throw new UnsupportedOperationException("Metrics are not supported by Flink");
+
+ // TODO: There is no use for timeBucketSizeInSecs yet.
+ if (metric instanceof FlinkCountMetric) {
--- End diff --
Here it looks user need to change code to make metrics work
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on the issue:
https://github.com/apache/flink/pull/1157
Here it looks user need to change code to make metrics work
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the issue:
https://github.com/apache/flink/pull/1157
@RalphSu We should wait for @HuangWHWHW to reply... If no response comes within 3 days you can take over. Also, the JIRA should get assigned to you for this case: https://issues.apache.org/jira/browse/FLINK-2720
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the issue:
https://github.com/apache/flink/pull/1157
Did not have a chance to work on this either -- I am in a new job, too :)
You can pick this up again if you are still interested.
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on the issue:
https://github.com/apache/flink/pull/1157
Here it looks user need to change code to make metrics work
---
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 #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMe...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on a diff in the pull request:
https://github.com/apache/flink/pull/1157#discussion_r104311095
--- Diff: flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkTopologyContext.java ---
@@ -120,7 +129,19 @@ public ReducedMetric registerMetric(final String name, final IReducer combiner,
@SuppressWarnings("unchecked")
@Override
public IMetric registerMetric(final String name, final IMetric metric, final int timeBucketSizeInSecs) {
- throw new UnsupportedOperationException("Metrics are not supported by Flink");
+
+ // TODO: There is no use for timeBucketSizeInSecs yet.
+ if (metric instanceof FlinkCountMetric) {
--- End diff --
Here it looks user need to change code to make metrics work
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-152474074
Hi. Are you still working on this? It's a couple of weeks since the last update.
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on the issue:
https://github.com/apache/flink/pull/1157
?
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the issue:
https://github.com/apache/flink/pull/1157
@HuangWHWHW Can you please close this PR? @RalphSu is working on this now (cf #3615 ). Thanks.
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-148370496
Any progress on this?
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-158127308
@HuangWHWHW What is the state of this PR? If you don't want to finish this, please close the PR (you can reopen a new PR any time later if you start to work on it again -- or are you still on it?)
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on the issue:
https://github.com/apache/flink/pull/1157
okey, would try file a PR on this soon
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by HuangWHWHW <gi...@git.apache.org>.
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-146387184
Hi, sorry for late reply.
The `FlinkCounMetric` is a wrapper for the flink-accumulator.
But I think I get your idea.Maybe I can remove the `FlinkCounMetric`.
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by HuangWHWHW <gi...@git.apache.org>.
Github user HuangWHWHW commented on the issue:
https://github.com/apache/flink/pull/1157
@mjsax Hi, very sorry to hang you out. I just did some other jobs last year because of some special circumstances.
How is the progress been recently?
Please let me know if you need some help here.
Very sorry before.
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-148658081
There is no need to rush. I was just curious. :)
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by HuangWHWHW <gi...@git.apache.org>.
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-148657366
@mjsax
Ah, very sorry to keep you wait.
Due to the National Day, I was on the vocation last week and I get some hard jobs this week.
I'll rework this during this weekend and will reply to you next Monday.
:-)
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1157#discussion_r39972364
--- Diff: flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkTopologyContext.java ---
@@ -53,6 +58,10 @@ public FlinkTopologyContext(final StormTopology topology, final Map<Integer, Str
null, null);
}
+ public void setContext(final StreamingRuntimeContext context) {
--- End diff --
I would extend the constructor instead of `setContext`. Using the constructor is less error prone because it forced to provide the `RuntimeContext`. An additional call to `setContext` could be "forgotten" by a programmer.
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-171352040
As it seems you are not working on this any longer, I take over the JIRA. Please close this PR. Otherwise, I will close it the next days.
---
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-2720][storm-compatibility]Add Storm-Cou...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1157#issuecomment-142210957
Yes, but I don't understand the purpose of `FlinkCounMetric`... Flink should be able to deal with Storm metrics directly (without requiring the user to use a special Flink metric class).
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by RalphSu <gi...@git.apache.org>.
Github user RalphSu commented on the issue:
https://github.com/apache/flink/pull/1157
Any progress on this one? Looks this has been stop for a while.
---
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 issue #1157: [FLINK-2720][storm-compatibility]Add Storm-CountMetric fo...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the issue:
https://github.com/apache/flink/pull/1157
@HuangWHWHW Are you still working on this? @RalphSu Are you interested in talking this over?
---
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.
---