You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tzulitai <gi...@git.apache.org> on 2017/08/14 07:43:29 UTC
[GitHub] flink pull request #4537: [FLINK-7440] [kinesis] Add various eager serializa...
GitHub user tzulitai opened a pull request:
https://github.com/apache/flink/pull/4537
[FLINK-7440] [kinesis] Add various eager serializability checks on to Kinesis connector
## What is the purpose of the change
This is a minor improvement for user experience for the Kinesis consumer and producer.
If the provided `KinesisDeserializationSchema`, `KinesisSerializationSchema`, `KinesisPartitioner` is not serializable, a more clear error message is shown instead of some vague "the implementation of RichSunkFunction is not serializable".
## Brief change log
Changes are broken up into 2 commits:
- for consumer: eagerly check serializability of `KinesisDeserializationSchema`, with better error msgs
- for producer: eagerly check serializability of `KinesisSerializationSchema` and `KinesisPartitioner`, with better error msgs.
## Verifying this change
Added new tests to both `FlinkKinesisConsumerTest` and `FlinkKinesisProducer` test, to verify that the expected error message is thrown (and also not thrown if the provided arg instances are serializable). Also included tests to check that `FlinkKinesisConsumer` and `FlinkKinesisProducer` themselves are serializable.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **no**
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: **no**
- The serializers: **no**
- The runtime per-record code paths (performance sensitive): **no**
- Anything that affects deployment or recovery: **no**
## Documentation
- Does this pull request introduce a new feature? **no**
- If yes, how is the feature documented? **not applicable**
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tzulitai/flink FLINK-7440
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4537.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 #4537
----
commit 96f0799aaece6ae226aeef17ff96c2fe2debc3e0
Author: Tzu-Li (Gordon) Tai <tz...@apache.org>
Date: 2017-08-14T07:16:54Z
[FLINK-7440] [kinesis] Eagerly check serializability of deserialization schema in FlinkKinesisConsumer
This commit also adds tests for verifying that the FlinkKinesisConsumer
itself is serializable.
commit 9faee3dc530c9b9021e33121f502f1e67deceaf0
Author: Tzu-Li (Gordon) Tai <tz...@apache.org>
Date: 2017-08-14T07:35:43Z
[FLINK-7440] [kinesis] Eagerly check that provided schema and partitioner is serializable in FlinkKinesisProducer
This commit also adds a test to verify that the FlinkKinesisProducer is
serializable.
----
---
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 #4537: [FLINK-7440] [kinesis] Add various eager serializa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4537
---
[GitHub] flink pull request #4537: [FLINK-7440] [kinesis] Add various eager serializa...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4537#discussion_r132999950
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/FlinkKinesisConsumerTest.java ---
@@ -1062,4 +1104,28 @@ public boolean isClearCalled() {
return fakeRestoredState;
}
+
+ private final class NonSerializableDeserializationSchema implements KinesisDeserializationSchema<String> {
--- End diff --
Can you please add a comment explaining why this is not serializable and why the following one is serializable? Basically pointing out this class is not static.
---
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 #4537: [FLINK-7440] [kinesis] Add various eager serializa...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4537#discussion_r132932684
--- Diff: flink-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/FlinkKinesisConsumer.java ---
@@ -176,6 +177,10 @@ public FlinkKinesisConsumer(List<String> streams, KinesisDeserializationSchema<T
// check the configuration properties for any conflicting settings
KinesisConfigUtil.validateConsumerConfiguration(this.configProps);
+ checkArgument(
--- End diff --
How about first checking for null, and then for serializability? (Just seem more intuitive to me)
---
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 #4537: [FLINK-7440] [kinesis] Add various eager serializability ...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4537
@tzulitai LGTM!
---
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 #4537: [FLINK-7440] [kinesis] Add various eager serializability ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4537
@bowenli86 @zentol Thanks a lot for the reviews! I will address your comments and merge 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 issue #4537: [FLINK-7440] [kinesis] Add various eager serializability ...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4537
Once this is checked in, we'll be more confident that https://github.com/apache/flink/pull/4473 is all good and it doesn't break anything
---
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 #4537: [FLINK-7440] [kinesis] Add various eager serializability ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4537
R: @bowenli86 would you like to take a look at the changes here?
---
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.
---