[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user bowenli86 commented on the issue: https://github.com/apache/flink/pull/5356 hmmm I think you are right, this actually might be a non-issue in the first place ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5356 I am wondering whether this discussion is a bit confused. All state facing the user in the APIs already has the behavior that there is no `null`, but only empty iterators. That's because all state is wrapped into a `UserFacingListState` in the `DefaultKeyedStateStore`. So, is this a non-issue, actually? Something that may only affect test implementations of `ListState` ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user bowenli86 commented on the issue: https://github.com/apache/flink/pull/5356 @StefanRRichter I think how `null` is handled is one major benefit, not all. Another benefit is that `iterator()` is more intuitive in traversing all values. For example, this change makes ListState more similar to how MapState handles traversing - In MapState, it has `values()` and `iterator()` that both traverse entries in the map. ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5356 I still do not believe that this is a significant improvement, and may even increase confusion: Now you have two slightly different ways to do almost the same thing, in one case you need to take care of `null` and in the other case not. It is always nicer to keep the core user interfaces slim. I would prefer to not include this change, and remember that this should be fixed if we come to the point of a API-breaking release. ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user bowenli86 commented on the issue: https://github.com/apache/flink/pull/5356 I also agree that changing `get()` API may not be good because it will break user's logic. From a user point of view, I found that receiving `null` from `get()` API is a bit inelegant and confusing. As a user, instinctively, I'd expect an empty Iterable when there's no state value. In practice, I had to debug several times and ended up finding the true semantics of `get()`. This is where `iterator()`'s value lies - to help users avoid such cases. Frankly, the `iterator()` API is not hard to maintain at all. It only invokes `get()`, handles the `null` situation, and returns users an empty iterable. ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5356 Ok, that is not so nice IMHO for an API, but if you think this might break some code then we have to keep it. Nevertheless, I still doubt about any additional value from the new method that is introduced by this PR. If we cannot change the old method, then I do not see a big benefit and this might not be worth having more methods in the API to maintain. What do you think @fhueske and @aljoscha ? ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/5356 @StefanRRichter that would be an (IMO unnecessary) API breaking change. See the comments on FLINK-8364. ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5356 I wonder why we need to introduce a separate method for an iterator instead of just changing the `get()` method to return `Collections.emptyList()` as iterable in case of null value. Wouldn't that be more straight forward? ---
[GitHub] flink issue #5356: [FLINK-8364][state backend] Add iterator() to ListState w...
Github user bowenli86 commented on the issue: https://github.com/apache/flink/pull/5356 cc @fhueske @aljoscha ---
