masteryhx commented on code in PR #23441:
URL: https://github.com/apache/flink/pull/23441#discussion_r1382731932
##########
flink-core/src/main/java/org/apache/flink/api/common/state/AppendingState.java:
##########
@@ -58,10 +58,11 @@ public interface AppendingState<IN, OUT> extends State {
* of values. The next time {@link #get()} is called (for the same state
partition) the returned
* state will represent the updated list.
*
- * <p>If null is passed in, the state value will remain unchanged.
+ * <p>If null is passed in, the behaviour is undefined (implementation
related).
Review Comment:
Could we unify the behavious of all sub classes?
I think "undefined" maybe also confuse users.
##########
flink-core/src/main/java/org/apache/flink/api/common/state/ListState.java:
##########
@@ -48,10 +48,13 @@ public interface ListState<T> extends MergingState<T,
Iterable<T>> {
* given list of values. The next time {@link #get()} is called (for the
same state partition)
* the returned state will represent the updated list.
*
- * <p>If null or an empty list is passed in, the state value will be null.
+ * <p>If an empty list is passed in, the state value will be null.
Review Comment:
Maybe it's also better to unify the behavious of null and empty list in the
future.
But this may modify the semantic of public API so it could be done in 2.0.
WDYT?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/state/PartitionableListState.java:
##########
@@ -122,15 +124,26 @@ public long[] write(FSDataOutputStream out) throws
IOException {
@Override
public void update(List<S> values) {
+ checkNotNull(values, "List of values to add cannot be null.");
+
internalList.clear();
- addAll(values);
+ if (!values.isEmpty()) {
Review Comment:
Could we just also use `addAll` here ?
##########
flink-core/src/main/java/org/apache/flink/api/common/state/AppendingState.java:
##########
@@ -58,10 +58,11 @@ public interface AppendingState<IN, OUT> extends State {
* of values. The next time {@link #get()} is called (for the same state
partition) the returned
* state will represent the updated list.
*
- * <p>If null is passed in, the state value will remain unchanged.
+ * <p>If null is passed in, the behaviour is undefined (implementation
related).
*
* @param value The new value for the state.
- * @throws Exception Thrown if the system cannot access the state.
+ * @throws Exception Thrown if the system cannot access the state, or null
value is passed to
Review Comment:
This seems conflict with last "undefined".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
