[GitHub] flink pull request: Add support for Subclasses, Interfaces, Abstra...
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/236#discussion_r24402955
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java ---
@@ -169,4 +186,113 @@ public ExecutionConfig disableObjectReuse() {
public boolean isObjectReuseEnabled() {
return objectReuse;
}
+
+ //
+ // Registry for types and serializers
+ //
+
+ /**
+* Registers the given Serializer as a default serializer for the given
type at the
--- End diff --
I think it would be better to call this method `registerTypeWithSerializer`.
defaultSerializers are something different in Kryo.
I'm sure users who know Kryo are confused by this Registers the given
Serializer as a default serializer for the given type at the
---
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: Add support for Subclasses, Interfaces, Abstra...
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/236#discussion_r24402907
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/runtime/KryoSerializer.java
---
@@ -82,29 +75,15 @@
//
- public KryoSerializer(ClassT type){
+ public KryoSerializer(ClassT type, ExecutionConfig executionConfig){
if(type == null){
throw new NullPointerException(Type class cannot be
null.);
}
this.type = type;
- // create copies of the statically registered serializers
- // we use static synchronization to safeguard against
concurrent use
- // of the static collections.
- synchronized (KryoSerializer.class) {
- this.registeredSerializers =
staticRegisteredSerializers.isEmpty() ?
- Collections.Class?, Serializer?emptyMap()
:
- new HashMapClass?,
Serializer?(staticRegisteredSerializers);
-
- this.registeredSerializersClasses =
staticRegisteredSerializersClasses.isEmpty() ?
- Collections.Class?, Class? extends
Serializer?emptyMap() :
- new HashMapClass?, Class? extends
Serializer?(staticRegisteredSerializersClasses);
-
- this.registeredTypes = staticRegisteredTypes.isEmpty() ?
- Collections.Class?emptySet() :
- new HashSetClass?(staticRegisteredTypes);
- }
-
+ this.registeredSerializers =
executionConfig.getRegisteredKryoSerializers();
--- End diff --
We should add a way to register default Kryo serializers as well .. to have
complete support.
---
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: Add support for Subclasses, Interfaces, Abstra...
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/236#discussion_r24403182
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java ---
@@ -169,4 +186,113 @@ public ExecutionConfig disableObjectReuse() {
public boolean isObjectReuseEnabled() {
return objectReuse;
}
+
+ //
+ // Registry for types and serializers
+ //
+
+ /**
+* Registers the given Serializer as a default serializer for the given
type at the
--- End diff --
yes :dancers:
---
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: Add support for Subclasses, Interfaces, Abstra...
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/236#discussion_r24403132
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java ---
@@ -169,4 +186,113 @@ public ExecutionConfig disableObjectReuse() {
public boolean isObjectReuseEnabled() {
return objectReuse;
}
+
+ //
+ // Registry for types and serializers
+ //
+
+ /**
+* Registers the given Serializer as a default serializer for the given
type at the
--- End diff --
If you want I can also change it. I've another change for the Kryo system
pending, so I can do the change in the course of 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: Add support for Subclasses, Interfaces, Abstra...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-72432067 That is not a really good solution, IMHO. Can we have a special tag that signifies that the deserialization is delegated to Kryo? How about this flow: - If class is `null`, then write null tag. - If class is original class, write same class tag - If class is registered and pojo, handle with pojo logic (tag and write class) - If class is registered and not pojo, write kryo tag and delegate to kryo - If class is not registered, write kryo tag and delegate to kryo --- 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: Add support for Subclasses, Interfaces, Abstra...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-72433217 Yes, it is not a good solution But what you propose isn't either: If we use Kryo for those subclasses that we cannot handle then nothing works anymore. The whole reason we have support for POJOs is that we can theoretically compare them in their binary representation. We are not doing this right now (the PojoComparator is always comparing in deserialised form) but we added it with that goal in mind. If we don't want to do that anymore we can just get rid of the whole POJO serialisation code and use Kryo for everything. --- 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: Add support for Subclasses, Interfaces, Abstra...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-72434125 Dang, I see your point. We need something more clever then... Can we (for now) at least make sure that it fails early: Get an exception when registering a subtype of a pojo at the pojo serializer (in the pre-flight phase)? And give a pointer to the flag that forces Kryo Serialization? --- 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: Add support for Subclasses, Interfaces, Abstra...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-72103112 What would happen if a base class fulfills the POJO requirements, but the subclass does not? --- 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: Add support for Subclasses, Interfaces, Abstra...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-72161285 Then it fails at runtime, which makes me very uneasy. But then again, stuff can always fail at runtime when the user uses some strange subclass. Even more so without this 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: Add support for Subclasses, Interfaces, Abstra...
Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-71436626 @aljoscha Have a look at #316 where I took this PR, rebased it, and fixed some problems with Pojo 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: Add support for Subclasses, Interfaces, Abstra...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-71436404 I will have to rework this now that the support for registering Types and Serializers at Kryo was merged. The POJO subclass with tagging is slower because we do additional checks and lookups: Upon serialisation we perform a map lookup to check whether the subclass is actually a registered class. When deserialising we have to fetch the correct subclass serialiser from an array of subclass serialisers. --- 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: Add support for Subclasses, Interfaces, Abstra...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-70413969 Two questions relevant to the interplay with other code: - How does the `registerType()` interact with the registration of custom kryo serializers? - Why is the POJO subclass with tagging significantly slower than the POJO base class here? I thought that both cases are tagged (as being base class or subclass). What causes the time difference 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: Add support for Subclasses, Interfaces, Abstra...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-70095528 I would like to have this merged soon. It contains some good changes to the TypeExtractor (support for interfaces abstract classes) --- 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: Add support for Subclasses, Interfaces, Abstra...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/236#issuecomment-70216890 No objections, your honour. --- 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. ---
