You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by knaufk <gi...@git.apache.org> on 2016/05/10 21:41:23 UTC
[GitHub] flink pull request: [Flink 3758] Add possibility to register accum...
GitHub user knaufk opened a pull request:
https://github.com/apache/flink/pull/1979
[Flink 3758] Add possibility to register accumulators in custom triggers 1 of 1
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](http://flink.apache.org/how-to-contribute.html).
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
- 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/knaufk/flink FLINK-3758
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1979.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 #1979
----
commit d093fdd6bd22382cde3ae202adfd6d407caac0e8
Author: Konstantin Knauf <ko...@tngtech.com>
Date: 2016-05-03T20:45:48Z
Added getOrDefaultAccumulator to RuntimeContext
commit 8fc4d39bbb5c76a1b0cc0a4e93522e59fb2135bd
Author: Konstantin Knauf <ko...@tngtech.com>
Date: 2016-05-03T20:46:17Z
Exposed getOrDefaultAccumulator in WindowContext
commit 08df4fb00a500f8a131b4364f94c264c0b51e6b8
Author: Konstantin Knauf <ko...@tngtech.com>
Date: 2016-05-05T16:42:10Z
Added simple test for Accumulators in Triggers
----
---
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-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1979#issuecomment-218805483
Nice and straightforward, I just had some minor comments about formatting. \U0001f604
And of course we protect the feature with a test.
If you want I can fix the formatting while merging. Or you can fix 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 pull request: [FLINK-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1979#issuecomment-218817622
R: @rmetzger
R: @StephanEwen
What do you think about the interface. This adds a method to `RuntimeContext`. This does not break if users are only using it. It would break if someone implements 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 pull request: [FLINK-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1979#discussion_r63047584
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/triggers/Trigger.java ---
@@ -169,7 +170,17 @@ public void clear(W window, TriggerContext ctx) throws Exception {}
* function (function is not part os a KeyedStream).
*/
<S extends State> S getPartitionedState(StateDescriptor<S, ?> stateDescriptor);
-
+
+ /**
+ *
--- End diff --
extra line
---
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-3758] Add possibility to register accumulators in...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1979
How about tagging the new method with `@PublicEvolving`. Since its a new API anyways, its good not to make it part of the stable interfaces.
With Flink 2.0, we can remove the `Evolving` part ;)
---
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-3758] Add possibility to register accum...
Posted by knaufk <gi...@git.apache.org>.
Github user knaufk commented on the pull request:
https://github.com/apache/flink/pull/1979#issuecomment-218815109
Hopefully, fixed the formatting stuff. If there are still missing lines or so, please just go ahead and fix it during merging...
---
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-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1979#issuecomment-218820979
No one should implement it. With interfaces it's hard to now what the stance is, that's why I'm including those two. @rmetzger implemented the tool that will check whether releases break API compatibility and @StephanEwen always has an eye on these.
---
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-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1979#discussion_r63047435
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/functions/util/AbstractRuntimeUDFContext.java ---
@@ -43,9 +38,13 @@
import org.apache.flink.api.common.state.ValueStateDescriptor;
import org.apache.flink.api.common.typeinfo.TypeInformation;
import org.apache.flink.core.fs.Path;
-
import static org.apache.flink.util.Preconditions.checkNotNull;
+import java.io.Serializable;
--- End diff --
unrelated changes. the order of imports should stay the same, even though it might be arbitrary \U0001f603
---
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-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1979#discussion_r63047546
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/functions/util/AbstractRuntimeUDFContext.java ---
@@ -133,13 +132,24 @@ public DoubleCounter getDoubleCounter(String name) {
accumulators.put(name, accumulator);
}
- @SuppressWarnings("unchecked")
--- End diff --
This should stay to prevent warnings, should also be added to the other `getAccumulator` method.
---
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-3758] Add possibility to register accumulators in...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1979
Sorry for chiming in late.
I was wondering whether we actually need accumulators here, or what we should add is access to the metrics. Because the use case seems to be "monitoring", not "side aggregates".
Admittedly, the accumulators were the means of choice for metrics up to now, but for the future (now that we are adding proper metrics), we may want to fix 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-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1979#discussion_r63047675
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/triggers/Trigger.java ---
@@ -169,7 +170,17 @@ public void clear(W window, TriggerContext ctx) throws Exception {}
* function (function is not part os a KeyedStream).
*/
<S extends State> S getPartitionedState(StateDescriptor<S, ?> stateDescriptor);
-
+
+ /**
+ *
+ * Get an existing accumulator by <code>name</code> if it exists, Otherwise add <code>defaultAccumulator</code>.
+ *
+ * @param name Name of the accumulator
+ * @param defaultAccumulator Accumulator to add, if no accumulator of the given name exists
+ * @return The accumulator registered by the given <code>name</code> or the given default <code>defaultAccumulator</code>
+ */
+ public <V, A extends Serializable> Accumulator<V, A> getAccumulator(String name, org.apache.flink.api.common.accumulators.Accumulator<V, A> defaultAccumulator);
--- End diff --
`org.apache.flink.api.common.accumulators.Accumulator` can be shortened to `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-3758] Add possibility to register accum...
Posted by knaufk <gi...@git.apache.org>.
Github user knaufk closed the pull request at:
https://github.com/apache/flink/pull/1979
---
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 #1979: [FLINK-3758] Add possibility to register accumulat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1979
---
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-3758] Add possibility to register accum...
Posted by knaufk <gi...@git.apache.org>.
GitHub user knaufk reopened a pull request:
https://github.com/apache/flink/pull/1979
[FLINK-3758] Add possibility to register accumulators in custom triggers
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](http://flink.apache.org/how-to-contribute.html).
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
- 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/knaufk/flink FLINK-3758
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1979.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 #1979
----
commit d093fdd6bd22382cde3ae202adfd6d407caac0e8
Author: Konstantin Knauf <ko...@tngtech.com>
Date: 2016-05-03T20:45:48Z
Added getOrDefaultAccumulator to RuntimeContext
commit 8fc4d39bbb5c76a1b0cc0a4e93522e59fb2135bd
Author: Konstantin Knauf <ko...@tngtech.com>
Date: 2016-05-03T20:46:17Z
Exposed getOrDefaultAccumulator in WindowContext
commit 08df4fb00a500f8a131b4364f94c264c0b51e6b8
Author: Konstantin Knauf <ko...@tngtech.com>
Date: 2016-05-05T16:42:10Z
Added simple test for Accumulators in Triggers
----
---
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-3758] Add possibility to register accum...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1979#issuecomment-218816900
Ok, thanks! I'm merging 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 pull request: [FLINK-3758] Add possibility to register accum...
Posted by knaufk <gi...@git.apache.org>.
Github user knaufk commented on the pull request:
https://github.com/apache/flink/pull/1979#issuecomment-218819781
I was worried about this to and intially only added the method to the AbstractRuntimeUDFContext. On the other hand, are you aware of anyone who has implemented RuntimeContext?
---
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 #1979: [FLINK-3758] Add possibility to register accumulators in ...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/1979
@knaufk Could you please close this now that the Jira issue is also closed?
---
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.
---