dianfu commented on code in PR #29576:
URL: https://github.com/apache/flink/pull/29576#discussion_r4373524329
##########
flink-python/pyflink/fn_execution/state_impl.py:
##########
@@ -815,8 +815,8 @@ def contains(self, map_key):
return True
def is_empty(self):
- if self._is_empty is None:
- if len(self._write_cache) > 0:
+ if self._is_empty is None or (not self._read_cache_enabled and not
self._is_empty):
Review Comment:
Could we preserve active MapState iterators when `is_empty()` flushes
pending mutations? This change introduces a regression with `OnReadAndWrite +
ReturnExpiredIfNotCleanedUp` when write caching is enabled.
For a fresh MapState with a one-day TTL and a write-cache capacity greater
than two entries:
```python
state.put(1, 100)
state.put(2, 200)
iterator = iter(state.items())
next(iterator) # (1, 100)
state.is_empty() # False
next(iterator) # Raises "Concurrent modification detected"
```
Expected: the last call returns `(2, 200)`. All writes precede iterator
creation, and only the read-only `is_empty()` call occurs between iterations.
No expiration is required.
The new branch calls `commit()`, which clears the `_write_cache` dictionary
backing the iterator and increments `_mod_count`. Consequently, the next
iteration fails. The same issue affects `keys()` and `values()`.
Could we make the emptiness check preserve active iterators while
maintaining the pending-write/clear semantics? For example, determine emptiness
from pending mutations where possible and ensure any required flush is safe for
existing iterators. Simply avoiding the `_mod_count` increment would not be
sufficient because `commit()` also clears the dictionary being iterated. Please
add regression coverage for this sequence and for `iterator.remove() →
is_empty() → next(iterator)`.
--
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]
