Hi Zakelly, Thanks for the quick response.
> 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? I'd prefer `completedVoidFuture` for consistency. > Yes, this will be added in implementation, I just omitted them for easy > reading. I think the JavaDoc is as important as the method itself, so it's better we also review the JavaDoc as part of the API. Best regards, Xuannan On Mon, Mar 11, 2024 at 5:34 PM Zakelly Lan <zakelly....@gmail.com> wrote: > > 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 > >> > >
