Hi Xuannan, Thanks for your comments!
1. The name `emptyFuture` seems a little unintuitive, and it is hard > to understand in what use case the `emptyFuture` should be used. If I > understand correctly, it is similar to the > FutureUtils#completedVoidFuture. How about naming it > completedVoidStateFuture? It will be used in callback chaining cases where some branch within one callback does nothing. I'm in favor of short phrases to express the functionalities. Thus I suggest `completedVoidFuture` or `voidFuture`, WDTY? 2. IIUC, the `FutureUtils` is intended to be used by the user. If > that's the case, `FutureUtils` should be annotated as a public > interface, such as `PublicEvolving`. > Yes I missed that, thanks for the reminder. 3. The state classes, such as `ValueState`, `ListState`, etc., are > essential for users, and we should add JavaDocs to those classes and > their methods. Yes, this will be added in implementation, I just omitted them for easy reading. Thanks & Best, Zakelly On Mon, Mar 11, 2024 at 5:25 PM Zakelly Lan <zakelly....@gmail.com> wrote: > Hi Yunfeng, > > Thanks for your comments! > > +1 for JingGe's suggestion to introduce an AsyncState API, instead of >> having both get() and asyncGet() in the same State class. As a >> supplement to its benefits, this design could help avoid having users >> to use sync and async API in a mixed way (unless they create both a >> State and an AsyncState from the same state descriptor), which is >> supposed to bring suboptimal performance according to the FLIP's >> description. > > > Actually splitting APIs into two sets of classes also brings some > difficulties. In this case, users must explicitly define their usage before > actually doing state access. It is a little strange that the user can > define a sync and an async version of State with the same name, while they > cannot allocate two async States with the same name. > Another reason for distinguishing API by their method name instead of > class name is that users typically use the State instances to access state > but forget their type/class. For example: > ``` > SyncState a = getState(xxx); > AsyncState b = getAsyncState(xxx); > //... > a.update(1); > b.update(1); > ``` > Users are likely to think there is no difference between the `a.update(1)` > and `b.update(1)`, since they may forget the type for `a` and `b`. Thus I > proposed to distinguish the behavior in method names. > As for the suboptimal performance with mixed usage of sync and async, my > proposal is to warn them in runtime. > > I noticed that the FLIP proposes to place the newly introduced API in >> the package "org.apache.flink.api.common.state.v2", which seems a >> little strange to me as there has not been such a naming pattern >> ".v2." for packages in Flink. > > > In fact, there are some similar existing patterns, like > `org.apache.flink.streaming.api.functions.sink.v2` and > `org.apache.flink.streaming.api.connector.sink2`. > > I would suggest discussing this topic >> with the main authors of Datastream V2, like Weijie Guo, so that the >> newly introduced APIs from both sides comply with a unified naming >> style. > > I'm afraid we are facing a different situation with the Datastream V2. For > total reconstruction of Datastream API, it is big enough to build a > seperate module and keep good package names. While for state APIs, we > should stay in the flink-core(-api) module alongside with other > apis, currently I tend to compromise at the expense of naming style. > > > Looking forward to hearing from you again! > > Thanks & Best, > Zakelly > > On Mon, Mar 11, 2024 at 4:20 PM Yunfeng Zhou <flink.zhouyunf...@gmail.com> > wrote: > >> Hi Zakelly, >> >> Thanks for the proposal! The structure of the Async API generally >> looks good to me. Some comments on the details of the design are as >> follows. >> >> +1 for JingGe's suggestion to introduce an AsyncState API, instead of >> having both get() and asyncGet() in the same State class. As a >> supplement to its benefits, this design could help avoid having users >> to use sync and async API in a mixed way (unless they create both a >> State and an AsyncState from the same state descriptor), which is >> supposed to bring suboptimal performance according to the FLIP's >> description. >> >> I noticed that the FLIP proposes to place the newly introduced API in >> the package "org.apache.flink.api.common.state.v2", which seems a >> little strange to me as there has not been such a naming pattern >> ".v2." for packages in Flink. I would suggest discussing this topic >> with the main authors of Datastream V2, like Weijie Guo, so that the >> newly introduced APIs from both sides comply with a unified naming >> style. If we reach an agreement on the first comment, my personal idea >> is that we can place the AsyncState interfaces to >> "org.apache.flink.api.common.state.async", and the existing state APIs >> to "org.apache.flink.api.common.state" or >> "org.apache.flink.api.common.state.sync". >> >> Best regards, >> Yunfeng Zhou >> >> On Thu, Mar 7, 2024 at 4:48 PM Zakelly Lan <zakelly....@gmail.com> wrote: >> > >> > Hi devs, >> > >> > I'd like to start a discussion on a sub-FLIP of FLIP-423: Disaggregated >> > State Storage and Management[1], which is a joint work of Yuan Mei, >> Zakelly >> > Lan, Jinzhong Li, Hangxiang Yu, Yanfei Lei and Feng Wang: >> > >> > - FLIP-424: Asynchronous State APIs [2] >> > >> > This FLIP introduces new APIs for asynchronous state access. >> > >> > Please make sure you have read the FLIP-423[1] to know the whole story, >> and >> > we'll discuss the details of FLIP-424[2] under this mail. For the >> > discussion of overall architecture or topics related with multiple >> > sub-FLIPs, please post in the previous mail[3]. >> > >> > Looking forward to hearing from you! >> > >> > [1] https://cwiki.apache.org/confluence/x/R4p3EQ >> > [2] https://cwiki.apache.org/confluence/x/SYp3EQ >> > [3] https://lists.apache.org/thread/ct8smn6g9y0b8730z7rp9zfpnwmj8vf0 >> > >> > >> > Best, >> > Zakelly >> >
