You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by twalthr <gi...@git.apache.org> on 2014/11/26 21:10:12 UTC
[GitHub] incubator-flink pull request: canCreateInstance() method for type ...
GitHub user twalthr opened a pull request:
https://github.com/apache/incubator-flink/pull/239
canCreateInstance() method for type serializers
In order to move to the Kryo serializer for generic type serialization, a canCreateInstance() method is necessary. @tillrohrmann already did this in a private branch. I rebased, cherry-picked and reviewed it and updated the new serializers.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/twalthr/incubator-flink canCreateInstanceMethod
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-flink/pull/239.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 #239
----
commit 4825f2fce000744ead50368473eeb1022855edc7
Author: Till Rohrmann <tr...@apache.org>
Date: 2014-10-28T15:47:43Z
Added canCreateInstance method to type serializers.
Conflicts:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/runtime/KryoSerializer.java
commit cb075ffac35fd558685354c740f210bde020d478
Author: twalthr <in...@twalthr.com>
Date: 2014-11-26T20:08:01Z
Update remaining serializers
----
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/239#discussion_r20989217
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeutils/TypeSerializer.java ---
@@ -59,8 +59,14 @@
* @return True, if the serializer is stateful, false if it is stateless;
*/
public abstract boolean isStateful();
-
-
+
+
+ /**
+ * Gets whether an instance of the underlying type can be created by this serializer
--- End diff --
- I would remove the `@return`
- I think "Returns whether..." is better than "Gets whether..."
- Empty line above the beginning of the comment
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/incubator-flink/pull/239#issuecomment-64776347
The new functionality is not used anywhere? Or am I missing something?
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/239#discussion_r20989360
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/array/BooleanPrimitiveArraySerializer.java ---
@@ -45,10 +43,16 @@ public boolean isImmutableType() {
public boolean isStateful() {
return false;
}
-
+
+ @Override
+ public boolean canCreateInstance() {
+ return false;
+ }
+
@Override
public boolean[] createInstance() {
- return EMPTY;
+ throw new UnsupportedOperationException("BooleanPrimitiveArraySerializer cannot create an" +
--- End diff --
I think most of the project code is using 120 chars for line breaks, so you don't need the line break here if you don't want to
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/239#discussion_r20989471
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java ---
@@ -57,13 +59,16 @@ public PojoSerializer(Class<T> clazz, TypeSerializer<?>[] fieldSerializers, Fiel
}
boolean stateful = false;
+ boolean canCreateInstance = true;
for (TypeSerializer<?> ser : fieldSerializers) {
if (ser.isStateful()) {
--- End diff --
Might be better readable to have both checks in the same format, i.e. either both as branches or both ANDing.
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/239#discussion_r20989260
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/GenericArraySerializer.java ---
@@ -62,7 +62,12 @@ public boolean isStateful() {
return this.componentSerializer.isStateful();
}
-
+ @Override
+ public boolean canCreateInstance() {
+ return true;
+ }
+
+
--- End diff --
Empty 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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/incubator-flink/pull/239#issuecomment-64775542
The code changes look good to me, but some of the tests relied on calls to `createInstance`, which now throw an Exception, for example the `StringSerializer`.
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/incubator-flink/pull/239#issuecomment-64782389
No, but some tests use `StringSerializer.createInstance()`, which used to return `EMPTY`, but now throws an `Exception`.
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr closed the pull request at:
https://github.com/apache/incubator-flink/pull/239
---
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] incubator-flink pull request: canCreateInstance() method for type ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/incubator-flink/pull/239#issuecomment-65660873
Thanks for your feedback. I underestimated the impact and amount of work, I will close this PR and reopen a new one later.
---
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.
---