[jira] [Commented] (FLINK-8411) inconsistent behavior between HeapListState#add() and RocksDBListState#add()

[ASF GitHub Bot (JIRA)]

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

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

GitHub user bowenli86 opened a pull request:

https://github.com/apache/flink/pull/5300

[FLINK-8411] [State Backends] inconsistent behavior between 
HeapListState#add() and RocksDBListState#add()

## What is the purpose of the change

`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`

## Brief change log

- changed ListState impls such that `add(null)` will be explicitly ignored
- added unit tests to test `add(null)`
- updated javaDoc

## Verifying this change

This change is already covered by existing tests, such as 
`StateBackendTestBase`.

## Does this pull request potentially affect one of the following parts:

  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: yes

## Documentation

  - Does this pull request introduce a new feature? (no)
  - If yes, how is the feature documented? (JavaDocs)


Note! **This work depends on FLINK-7983 and its PR at 
https://github.com/apache/flink/pull/5281**

cc @StefanRRichter 

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bowenli86/flink FLINK-8411

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/5300.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 #5300


commit b4034f67e727fef68740221e3b31cd131c905df1
Author: Bowen Li 
Date:   2018-01-02T19:21:28Z

update local branch

commit 11e9c255cd51304c5281d55226fbb6fe8360d8a2
Author: Bowen Li 
Date:   2018-01-04T01:35:11Z

remove sh

commit 72edff2a82df625203289e4b5be23db36b03abe3
Author: Bowen Li 
Date:   2018-01-12T08:14:55Z

[FLINK-7938] introduce addAll() to ListState

commit 138d6f63dff7a840b716bf8900f97940f7d61dd8
Author: Bowen Li 
Date:   2018-01-12T18:56:42Z

add unit tests

commit 481a5a98b7b655658855e18023c7c28328b0b47d
Author: Bowen Li 
Date:   2018-01-12T19:03:07Z

add documentation for addAll()

commit cced3bac65660b27e17d258b3fd3880e9571bcf6
Author: Bowen Li 
Date:   2018-01-13T23:06:11Z

add more cases in unit test

commit 18fb3ff2965653dc0bd7c7a2d7a419ce8c7c6e8a
Author: Bowen Li 
Date:   2018-01-15T23:24:19Z

[FLINK-8411] inconsistent behavior between HeapListState#add() and 
RocksDBListState#add()




> inconsistent behavior between HeapListState#add() and RocksDBListState#add()
> 
>
> Key: FLINK-8411
> URL: https://issues.apache.org/jira/browse/FLINK-8411
> Project: Flink
>  Issue Type: Bug
>  Components: State Backends, Checkpointing
>Affects Versions: 1.4.0
>Reporter: Bowen Li
>Assignee: Bowen Li
>Priority: Critical
> Fix For: 1.5.0, 1.4.1
>
>
> 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 map = stateTable;
>   ArrayList 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}

[jira] [Commented] (FLINK-8411) inconsistent behavior between HeapListState#add() and RocksDBListState#add()

[Stefan Richter (JIRA)]

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

Stefan Richter commented on FLINK-8411:
---

Yes, I agree. Would you like to make a PR or should I just fix it for you?

> inconsistent behavior between HeapListState#add() and RocksDBListState#add()
> 
>
> Key: FLINK-8411
> URL: https://issues.apache.org/jira/browse/FLINK-8411
> Project: Flink
>  Issue Type: Bug
>Affects Versions: 1.4.0
>Reporter: Bowen Li
>Assignee: Bowen Li
>Priority: Critical
> Fix For: 1.5.0, 1.4.1
>
>
> 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 map = stateTable;
>   ArrayList 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
(v6.4.14#64029)