flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (FLINK-8411) HeapListState#add(null) will wipe out entire list state
Date Wed, 14 Feb 2018 18:44:00 GMT

    [ https://issues.apache.org/jira/browse/FLINK-8411?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16364588#comment-16364588
] 

ASF GitHub Bot commented on FLINK-8411:
---------------------------------------

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?


> HeapListState#add(null) will wipe out entire list state
> -------------------------------------------------------
>
>                 Key: FLINK-8411
>                 URL: https://issues.apache.org/jira/browse/FLINK-8411
>             Project: Flink
>          Issue Type: Improvement
>          Components: State Backends, Checkpointing
>    Affects Versions: 1.4.0
>            Reporter: Bowen Li
>            Assignee: Bowen Li
>            Priority: Blocker
>             Fix For: 1.5.0
>
>
> You can see that {{HeapListState#add(null)}} will result in the whole state being cleared
or wiped out. There's never a unit test for {{List#add(null)}} in {{StateBackendTestBase}}
> {code:java}
> // HeapListState
> @Override
> 	public void add(V value) {
> 		final N namespace = currentNamespace;
> 		if (value == null) {
> 			clear();
> 			return;
> 		}
> 		final StateTable<K, N, ArrayList<V>> map = stateTable;
> 		ArrayList<V> list = map.get(namespace);
> 		if (list == null) {
> 			list = new ArrayList<>();
> 			map.put(namespace, list);
> 		}
> 		list.add(value);
> 	}
> {code}
> {code:java}
> // RocksDBListState
> @Override
> 	public void add(V value) throws IOException {
> 		try {
> 			writeCurrentKeyWithGroupAndNamespace();
> 			byte[] key = keySerializationStream.toByteArray();
> 			keySerializationStream.reset();
> 			DataOutputViewStreamWrapper out = new DataOutputViewStreamWrapper(keySerializationStream);
> 			valueSerializer.serialize(value, out);
> 			backend.db.merge(columnFamily, writeOptions, key, keySerializationStream.toByteArray());
> 		} catch (Exception e) {
> 			throw new RuntimeException("Error while adding data to RocksDB", e);
> 		}
> 	}
> {code}
> The fix should correct the behavior to be consistent between the two state backends,
as well as adding a unit test for {{ListState#add(null)}}. For the correct behavior, I believe
adding null with {{add(null)}} should simply be ignored without any consequences.
> cc [~srichter]



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message