You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by aljoscha <gi...@git.apache.org> on 2018/02/14 13:07:40 UTC
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
GitHub user aljoscha opened a pull request:
https://github.com/apache/flink/pull/5485
[FLINK-8411] Don't allow null in ListState.add()/addAll()
R: @StefanRRichter, @bowenli86
It turns out that this is a bit trickier than assumed earlier: `ListState.addAll()` was not considered and also had inconsistent behaviour between state backends before.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aljoscha/flink jira-8411-fix-list-add
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5485.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 #5485
----
commit b39aa20d60df9effb536cc96c308645b9688113d
Author: Aljoscha Krettek <al...@...>
Date: 2018-02-14T11:04:20Z
[FLINK-8411] Don't allow null in ListState.add()/addAll()
----
---
[GitHub] flink issue #5485: [FLINK-8411] Don't allow null in ListState.add()/addAll()
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5485
@zentol pushed a fixup. What about the rest of the PR?
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168767915
--- Diff: flink-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBaseTest.java ---
@@ -696,6 +696,7 @@ public void clear() {
@Override
--- End diff --
And also, there is the `update()` method in the `ListState` interface that should be changed in all implementations.
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168765603
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/UserFacingListState.java ---
@@ -48,6 +49,7 @@
@Override
public void add(T value) throws Exception {
+ Preconditions.checkNotNull(value, "You cannot add null to a ListState.");
--- End diff --
You should also change the `addAll()` to check for `nulls` internally, as done in the `HeapListState`.
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168769781
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/UserFacingListState.java ---
@@ -48,6 +49,7 @@
@Override
public void add(T value) throws Exception {
+ Preconditions.checkNotNull(value, "You cannot add null to a ListState.");
--- End diff --
Yes, that would be cleaner.
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168271328
--- Diff: flink-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBaseTest.java ---
@@ -696,6 +697,7 @@ public void clear() {
@Override
public void add(T value) throws Exception {
+ Objects.requireNonNull(value, "You cannot add null to a ListState.");
--- End diff --
Why not our Preconditions version?
---
[GitHub] flink issue #5485: [FLINK-8411] Don't allow null in ListState.add()/addAll()
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/5485
LGTM
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168772337
--- Diff: flink-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBaseTest.java ---
@@ -696,6 +696,7 @@ public void clear() {
@Override
--- End diff --
fixing
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha closed the pull request at:
https://github.com/apache/flink/pull/5485
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168766013
--- Diff: flink-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBaseTest.java ---
@@ -696,6 +696,7 @@ public void clear() {
@Override
--- End diff --
Change also the `addAll()`.
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168769299
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/UserFacingListState.java ---
@@ -48,6 +49,7 @@
@Override
public void add(T value) throws Exception {
+ Preconditions.checkNotNull(value, "You cannot add null to a ListState.");
--- End diff --
Or not change this one, and defer null detection completely to the wrapping state. WDYT?
---
[GitHub] flink issue #5485: [FLINK-8411] Don't allow null in ListState.add()/addAll()
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5485
@kl0u Thanks for the review, and very good catch about `update()`!
---
[GitHub] flink pull request #5485: [FLINK-8411] Don't allow null in ListState.add()/a...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5485#discussion_r168414749
--- Diff: flink-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBaseTest.java ---
@@ -696,6 +697,7 @@ public void clear() {
@Override
public void add(T value) throws Exception {
+ Objects.requireNonNull(value, "You cannot add null to a ListState.");
--- End diff --
Will change
---