[GitHub] flink pull request: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/473 --- 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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/473#issuecomment-101010857 I maintain one comment, but I am not blocking this. Feel free 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/473#discussion_r30067549
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoTypeInfo.java
---
@@ -313,6 +319,15 @@ public int getFieldIndex(String fieldName) {
@Override
public TypeSerializerT createSerializer(ExecutionConfig config) {
+ if(config.isForceKryoEnabled()) {
+ LOG.debug(Using KryoSerializer for serializing POJOs);
--- End diff --
I still think this is the wrong place to log this, it will get repeated a
gazillion times on large plans. Also, it is logged in some places on the
client, and sometimes in the runtime.
It is not terribly bad, since it is debug level, but I don't get the reason
to have it in the first place. Since you have no context information (what
stream/dataset this is for), it is not a good debugging help anyways...
---
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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/473#discussion_r29937747
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/GenericTypeInfo.java
---
@@ -70,9 +69,6 @@ public boolean isKeyType() {
@Override
public TypeSerializerT createSerializer(ExecutionConfig config) {
- if(config.serializeGenericTypesWithAvro()) {
--- End diff --
POJOs generated by Avro will be handled by the Avro serializer
Generic Types always with Kryo serializer
This change allows to force the POJO serializer to serialize the ENTIRE
pojo with Kryo or Avro.
I added a section in the documentation explaining the settings.
---
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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/473#issuecomment-100226540 I rebased to the current master and addressed the concerns. --- 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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/473#discussion_r29936564
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoTypeInfo.java
---
@@ -310,6 +316,18 @@ public int getFieldIndex(String fieldName) {
@Override
public TypeSerializerT createSerializer(ExecutionConfig config) {
+ if(config.isForceKryoEnabled() config.isForceAvroEnabled()) {
--- End diff --
Okay. I'll move it to the other executionconfig related logging at the
executionenvironment.
---
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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/473#issuecomment-88540354 I reworked the PR. It is now ready for review again. --- 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: [FLINK-1676] Rework ExecutionConfig.enableForc...
GitHub user rmetzger opened a pull request: https://github.com/apache/flink/pull/473 [FLINK-1676] Rework ExecutionConfig.enableForceKryo() Please note that we are now using the PojoComparator with the GenericTypeInfo (Kryo or Avro) if ExecutionConfig.enableForceKryo() is set. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rmetzger/flink flink1676 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/473.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 #473 commit 1dfe0406dee2e4fda49c99399f48fa6dfa9beeca Author: Robert Metzger rmetz...@apache.org Date: 2015-03-10T16:22:46Z [FLINK-1676] Rework ExecutionConfig.enableForceKryo() --- 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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/473#discussion_r26194005
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoTypeInfo.java
---
@@ -310,6 +310,9 @@ public int getFieldIndex(String fieldName) {
@Override
public TypeSerializerT createSerializer(ExecutionConfig config) {
+ if(config.isForceKryoEnabled()) {
+ return new
GenericTypeInfoT(this.typeClass).createSerializer(config);
--- End diff --
I thought it makes sense to not have the switch, since the flag is called
forceKryo...
---
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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/473#discussion_r26156796
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoTypeInfo.java
---
@@ -310,6 +310,9 @@ public int getFieldIndex(String fieldName) {
@Override
public TypeSerializerT createSerializer(ExecutionConfig config) {
+ if(config.isForceKryoEnabled()) {
+ return new
GenericTypeInfoT(this.typeClass).createSerializer(config);
--- End diff --
I think it would be simpler to just do `return new
KryoSerializerT(this.typeClass, config)`
---
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: [FLINK-1676] Rework ExecutionConfig.enableForc...
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/473#discussion_r26170417
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoTypeInfo.java
---
@@ -310,6 +310,9 @@ public int getFieldIndex(String fieldName) {
@Override
public TypeSerializerT createSerializer(ExecutionConfig config) {
+ if(config.isForceKryoEnabled()) {
+ return new
GenericTypeInfoT(this.typeClass).createSerializer(config);
--- End diff --
Yes, that would save the creation of the GenericTypeInfo.
I did it like this because the createSerializer() method of the
GenericTypeInfo is again configurable, creating a Kryo or Avro serializer,
depending on the execution config.
But I can also pull the Kryo/Avro switch to the PojoTypeInfo if you think
that's nicer.
---
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.
---
