You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2016/03/23 11:13:55 UTC
[GitHub] flink pull request: [FLINK-2609] [streaming] auto-register types
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/1833
[FLINK-2609] [streaming] auto-register types
This PR attempts to register all streaming operator output types at kryo.
In contrast to the Batch API
* the registration is done when an operation is defined, opposed to after all operations are defined
* autoRegistration must be disabled before an operation is created to have an effect
* the deduplicator HashSet is a protected field of the DataStream class opposed to a local variable
* still not visible to users
* accessible by all DataStream's
* accessible by Non-DataStream streams (KeyedStream etc) as long as they are in the same package
* a separate superclass for all Streams is a good idea in the long run
Most operations use DataStream#transform(), as such most registrations will take place here. One exception are key-selectors, which aremysteriously hidden in the Keyed-/Joined-/CoGroupedStreams where()/equalTo() methods, where their registration takes place.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink 2609_register_types
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1833.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 #1833
----
commit 286a588464b18654eed342ba0d9cc2e92646eef7
Author: zentol <ch...@apache.org>
Date: 2016-03-23T09:58:25Z
[FLINK-2609] [streaming] auto-register types
----
---
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-2609] [streaming] auto-register types
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-205759442
The deduplicator set is only used to prevent infinite resursion for recursive types, correct? if so, can't we just pass an empty set into the 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-2609] [streaming] auto-register types
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-200748507
Since we don't have to execute anything we should be able to add a separate light-weight test for this. Already found the first failure...
---
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-2609] [streaming] auto-register types
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-205346744
i decided against the ExecutionEnvironment because i do not want to add a method (to access the deduplicator) in a user-facing class, when a user would never use 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-2609] [streaming] auto-register types
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1833#discussion_r57193823
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/CoGroupedStreams.java ---
@@ -94,6 +95,7 @@ public CoGroupedStreams(DataStream<T1> input1, DataStream<T2> input2) {
*/
public <KEY> Where<KEY> where(KeySelector<T1, KEY> keySelector) {
TypeInformation<KEY> keyType = TypeExtractor.getKeySelectorTypes(keySelector, input1.getType());
+ Serializers.recursivelyRegisterType(keyType, input1.getExecutionEnvironment().getConfig(), DataStream.deduplicator);
--- End diff --
missed that one...
---
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-2609] [streaming] auto-register types
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-205393404
I'm not sure if we can register the KeySelector types that way.
---
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-2609] [streaming] auto-register types
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1833#discussion_r57151588
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/CoGroupedStreams.java ---
@@ -94,6 +95,7 @@ public CoGroupedStreams(DataStream<T1> input1, DataStream<T2> input2) {
*/
public <KEY> Where<KEY> where(KeySelector<T1, KEY> keySelector) {
TypeInformation<KEY> keyType = TypeExtractor.getKeySelectorTypes(keySelector, input1.getType());
+ Serializers.recursivelyRegisterType(keyType, input1.getExecutionEnvironment().getConfig(), DataStream.deduplicator);
--- End diff --
Why is there no check for the auto type registration?
---
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-2609] [streaming] auto-register types
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-200329892
I wonder if we can simply test this by extending an existing data stream test with some custom/pojo types to see if they are properly registered in the EC.
---
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-2609] [streaming] auto-register types
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-205446161
Let's find a way then :-)
---
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-2609] [streaming] auto-register types
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-200793560
+1 to merge
---
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 #1833: [FLINK-2609] [streaming] auto-register types
Posted by zentol <gi...@git.apache.org>.
Github user zentol closed the pull request at:
https://github.com/apache/flink/pull/1833
---
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-2609] [streaming] auto-register types
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-205307922
Let's not merge this as is, it uses again a static de-duplicator, like the DataSet API used to (we recently eliminated that). These static utils to work around putting adding variable in the right place is an antipattern.
Why not have the registration de-duplicator in the ExecutionEnvironment? Since the ExecutionCondig is also one-per environment, and is always accessed through the environment, that seems like the way to go...
---
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-2609] [streaming] auto-register types
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1833#issuecomment-205380553
I think static variable is even more problematic.
Other possibility is to traverse the graph at the very end, and register at that point what is encountered.
---
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.
---