[FLINK-8364][state backend] Add iterator() to ListState which returns empty iterator when it has no value

[bowenli86]

What is the purpose of the change

Add iterator() to ListState which returns empty iterator when it has no value

Brief change log

Add iterator() to ListState which returns empty iterator when it has no value

Verifying this change

This change added tests and can be verified by extending StateBackendTestBase#testListStateAPIs()

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? (yes)
• If yes, how is the feature documented? (docs / JavaDocs)

[bowenli86]

cc @fhueske @aljoscha @StefanRRichter

[StefanRRichter]

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. Wouldn't that be more straight forward?

[fhueske]

@StefanRRichter that would be an (IMO unnecessary) API breaking change. See the comments on FLINK-8364.

[StefanRRichter]

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 ?

[bowenli86]

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.

[StefanRRichter]

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

[bowenli86]

@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. It's especially useful when things like FLINK-8297 is added.

[StephanEwen]

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?

[bowenli86]

hmmm.... I think you are right, this actually might be a non-issue in the first place