Hi everyone, It seems we're gradually reaching a consensus. So I would like to start a vote after 72 hours if there are no further discussions.
Please let me know if you have any concerns, thanks! Best, Zakelly On Sat, Oct 7, 2023 at 4:07 PM Zakelly Lan <zakelly....@gmail.com> wrote: > > Hi Jing, > > Sorry for the late reply! I agree with you that we do not expect users > to do anything with Flink and we won't "bother" them with those > exceptions. However, users can still catch the `Throwable` and perform > any necessary logging activities, similar to how they use Java > Collection interfaces. > > > Thanks for your insights! > > Best, > Zakelly > > On Thu, Sep 21, 2023 at 8:43 PM Jing Ge <j...@ververica.com.invalid> wrote: > > > > Fair enough! Thanks Zakelly for the information. Afaic, even users can do > > nothing with Flink, they still can do something in their territory, at > > least doing some logging and metrics stuff, or triggering some other > > services in their ecosystem. After all, the Flink jobs they build are part > > of their service component. It didn't change the fact that we are going to > > use the anti-pattern. Just because we didn't expect users to do > > anything with Flink, does not mean users don't expect to do something with > > the expected exception. Anyway, I am open to hearing different opinions. > > > > Best regards, > > Jing > > > > On Thu, Sep 21, 2023 at 7:02 AM Zakelly Lan <zakelly....@gmail.com> wrote: > > > > > Hi Martijn, > > > > > > Thanks for the reminder! > > > > > > This FLIP proposes a change to the state API that is annotated as > > > @PublicEvolving and targets version 1.19. I have clarified this in > > > the "Proposed Change" section of the FLIP. > > > > > > > > > Hi Jing, > > > > > > Thanks for sharing your thoughts! Here are my opinions: > > > > > > 1. The exceptions of the state API are usually treated as critical > > > ones. In other words, if anything goes wrong with state accessing, the > > > element processing cannot proceed and the job should fail. Flink users > > > may not know what to do when they encounter these exceptions. I > > > believe this is the main reason why we want to replace them with > > > unchecked exceptions. > > > 2. There have also been some further discussions[1][2] from Stephan > > > and Shixiaogang below the one you pointed out [3], and it seems they > > > come to an agreement to use unchecked exceptions. After reviewing the > > > entire discussion on that PR, I think their arguments are reasonable > > > given the use case. > > > > > > Looking forward to your feedback. > > > > > > > > > Best, > > > Zakelly > > > > > > [1] https://github.com/apache/flink/pull/3380#issuecomment-286807853 > > > [2] https://github.com/apache/flink/pull/3380#issuecomment-286932133 > > > [3] https://github.com/apache/flink/pull/3380#issuecomment-281631160 > > > > > > On Thu, Sep 21, 2023 at 1:27 AM Jing Ge <j...@ververica.com.invalid> > > > wrote: > > > > > > > > sorry, typo: It is a known "anti-pattern" instead of "ant-pattern" > > > > > > > > Best regards, > > > > Jing > > > > > > > > On Wed, Sep 20, 2023 at 7:23 PM Jing Ge <j...@ververica.com> wrote: > > > > > > > > > Hi Zakelly, > > > > > > > > > > Thanks for driving this topic. From good software engineering's > > > > > perspective, I have different thoughts: > > > > > > > > > > 1. The idea to get rid of all checked Exceptions and replace them with > > > > > unchecked Exceptions is a known ant-pattern: "Generally speaking, do > > > not > > > > > throw a RuntimeException or create a subclass of RuntimeException > > > simply > > > > > because you don't want to be bothered with specifying the exceptions > > > your > > > > > methods can throw." [1] Checked Exceptions mean expected exceptions > > > that > > > > > can help developers find a way to catch them and decide what to do. It > > > is > > > > > part of the public API signature that can help developers build robust > > > > > systems. We should not mix concepts and build expected exceptions with > > > > > unchecked Java Exception classes. > > > > > 2. The comment Stephan left [2] clearly pointed out that we should > > > avoid > > > > > using generic Java Exceptions, and "find some more 'specific' > > > exceptions > > > > > for the signature, like throws IOException or throws > > > StateAccessException." > > > > > So, the idea is to define/use specific checked Exception classes > > > instead of > > > > > using unchecked Exceptions. > > > > > > > > > > Looking forward to your thoughts. > > > > > > > > > > Best regards, > > > > > Jing > > > > > > > > > > > > > > > [1] > > > > > > > > https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html > > > > > [2] https://github.com/apache/flink/pull/3380#issuecomment-281631160 > > > > > > > > > > On Wed, Sep 20, 2023 at 4:52 PM Zakelly Lan <zakelly....@gmail.com> > > > wrote: > > > > > > > > > >> Hi Yanfei, > > > > >> > > > > >> Thanks for your reply! > > > > >> > > > > >> Yes, this FLIP aims to change all state-related exceptions to > > > > >> unchecked exceptions and remove all exceptions from the signature. So > > > > >> I believe we have come to an agreement to keep the interfaces simple. > > > > >> > > > > >> > > > > >> Best regards, > > > > >> Zakelly > > > > >> > > > > >> On Wed, Sep 20, 2023 at 2:26 PM Zakelly Lan <zakelly....@gmail.com> > > > > >> wrote: > > > > >> > > > > > >> > Hi Hangxiang, > > > > >> > > > > > >> > Thank you for your response! Here are my thoughts: > > > > >> > > > > > >> > 1. Regarding the exceptions thrown by internal interfaces, I > > > > >> > suggest > > > > >> > keeping them as checked exceptions. Since these exceptions will be > > > > >> > handled by the internal callers, it is meaningful to throw them as > > > > >> > checked ones. If we need to make changes to these classes, we can > > > > >> > create separate tickets alongside this FLIP. What are your thoughts > > > on > > > > >> > this? > > > > >> > 2. StateIOException is primarily thrown by file-based state like > > > > >> > RocksDB, while StateAccessException is more generic and can be > > > thrown > > > > >> > by heap states. Additionally, I believe there may be more > > > > >> > subclasses > > > > >> > of StateAccessException that we can add. We can consider this when > > > > >> > implementing. > > > > >> > 3. I would like to make this change in version 1.19. As mentioned > > > > >> > in > > > > >> > this FLIP, many users do not catch any exceptions since the element > > > > >> > processing function exposes the exception to the upper layer. > > > > >> > Therefore, the impact is manageable. And I completely agree that we > > > > >> > should clearly document this change in the next release notes. > > > > >> > > > > > >> > > > > > >> > Best regards, > > > > >> > Zakelly > > > > >> > > > > > >> > On Wed, Sep 20, 2023 at 12:35 PM Yanfei Lei <fredia...@gmail.com> > > > > >> wrote: > > > > >> > > > > > > >> > > Hi Zakelly, > > > > >> > > > > > > >> > > Thanks for bringing this up. +1 for reorganizing. > > > > >> > > > > > > >> > > IIUC, this proposal aims to change all state-related exceptions > > > > >> > > to > > > > >> > > unchecked exceptions. If users have caught checked exceptions > > > (such as > > > > >> > > IOException ) in their code, leaving the code as is would also > > > work. > > > > >> > > > > > > >> > > Is it possible not to put any exceptions in the signature of > > > > >> > > user-facing interfaces? As the proposal mentioned, users can do a > > > few > > > > >> > > things even if they catch the exceptions. Keeping the interface > > > simple > > > > >> > > may be easier to understand. > > > > >> > > > > > > >> > > > > > > >> > > Best, > > > > >> > > Yanfei > > > > >> > > > > > > >> > > Hangxiang Yu <master...@gmail.com> 于2023年9月19日周二 18:16写道： > > > > >> > > > > > > > >> > > > Hi, Zakelly. > > > > >> > > > > > > > >> > > > Thanks for the proposal. > > > > >> > > > > > > > >> > > > +1 for reorganizing exceptions of state interfaces which indeed > > > > >> confuses me > > > > >> > > > currently. > > > > >> > > > > > > > >> > > > From my experience, users usually omit these exceptions because > > > > >> they cannot > > > > >> > > > do much even if they catch the exceptions. > > > > >> > > > > > > > >> > > > I have some problems and suggestions, PTAL: > > > > >> > > > > > > > >> > > > 1. Could we also reorganize or add more state exceptions > > > (may be > > > > >> related > > > > >> > > > to other state interfaces/classes e.g. KeyedStateBackend) > > > into > > > > >> the > > > > >> > > > exception class diagrams ? Although these state-related > > > classes > > > > >> may not > > > > >> > > > be public, it could be better to consider them together to > > > make > > > > >> all > > > > >> > > > state-related exceptions clear. For example, we could > > > reorganize > > > > >> some > > > > >> > > > existing exceptions such as StateMigrationException, add > > > > >> > > > some > > > > >> exceptions > > > > >> > > > such as StateNotFoundException. > > > > >> > > > 2. Could you clarify or give an example about the extended > > > > >> relation > > > > >> > > > "StateAccessException -- StateIOException" ? When do we just > > > > >> return > > > > >> > > > StateAccessException instead of StateIOException or others ? > > > > >> > > > 3. Which version do you want to implement it in ? Since it > > > has > > > > >> to break > > > > >> > > > changes for users who have catched the IOException, If we > > > want > > > > >> to implement > > > > >> > > > it in 1.19, we must mark it very clearly in the release > > > note, or > > > > >> we should > > > > >> > > > make it in 2.0. > > > > >> > > > > > > > >> > > > > > > > >> > > > On Tue, Sep 19, 2023 at 5:08 PM Zakelly Lan < > > > zakelly....@gmail.com> > > > > >> wrote: > > > > >> > > > > > > > >> > > > > Hi everyone, > > > > >> > > > > > > > > >> > > > > I would like to initiate a discussion on FLIP-368, which > > > focuses > > > > >> on > > > > >> > > > > reorganizing the exceptions thrown in state interfaces [1]. > > > > >> > > > > > > > > >> > > > > Currently, we have identified several problems with the > > > exceptions > > > > >> > > > > thrown by state-related interfaces: > > > > >> > > > > 1. The exception types thrown by each interface are > > > > >> inconsistent. > > > > >> > > > > While most of the interfaces claim to throw `Exception`, the > > > > >> > > > > interfaces of `ValueState` throw `IOException`. Additionally, > > > the > > > > >> > > > > `State#clear()` never throws an exception. This can be > > > confusing > > > > >> for > > > > >> > > > > users. > > > > >> > > > > 2. The use of `Exception` or `IOException` as the thrown > > > > >> exception > > > > >> > > > > type is too generic and lacks specificity. > > > > >> > > > > 3. Users may not be able to handle these exceptions. In > > > cases > > > > >> where > > > > >> > > > > an exception occurs while accessing state, the job should > > > fail. > > > > >> This > > > > >> > > > > aligns more with the characteristic of *unchecked exceptions* > > > > >> instead > > > > >> > > > > of checked exceptions. > > > > >> > > > > > > > > >> > > > > To address these issues, we borrow the idea of throwing > > > unchecked > > > > >> > > > > exceptions in Java Collection API and propose the following > > > > >> changes in > > > > >> > > > > state-related exceptions: > > > > >> > > > > 1. Introduction of specific unchecked exception types for > > > > >> different > > > > >> > > > > reasons, providing users with more precise information about > > > the > > > > >> cause > > > > >> > > > > of the exception. > > > > >> > > > > 2. Removal of all checked exceptions from interface > > > signatures > > > > >> and > > > > >> > > > > instead, throwing newly introduced unchecked exceptions in > > > > >> > > > > the > > > > >> > > > > implementations. > > > > >> > > > > > > > > >> > > > > Please share your thoughts and suggestions regarding the > > > proposed > > > > >> > > > > changes. Thank you for your attention and support. > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > Best, > > > > >> > > > > Zakelly > > > > >> > > > > > > > > >> > > > > [1] FLIP-368: Reorganize the exceptions thrown in state > > > > >> interfaces, > > > > >> > > > > https://cwiki.apache.org/confluence/x/eZ2zDw > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > -- > > > > >> > > > Best, > > > > >> > > > Hangxiang. > > > > >> > > > > > > > >
