+1 for the proposal But "Since the signature of the public state API has been changed", I was wondering whether this would be more fittable in Flink 2.0, instead of 1.19?
WDYT? Best Yuan On Wed, Oct 11, 2023 at 4:34 PM David Radley <david_rad...@uk.ibm.com> wrote: > Hi Zakelly, > Thanks for making this clear for me. We should document the impact on the > user in the release notes, which will be a minimal rewrite and recompile of > any java using the old APIs. > I think it is a good point you make about if there are future > implementations that are > worth retrying (such as network access) – then there could be retries. I > agree we should not be trying to create code now for an implementation > consideration that is not there yet, > > +1 from me , > Kind regards, David. > > From: Zakelly Lan <zakelly....@gmail.com> > Date: Wednesday, 11 October 2023 at 04:25 > To: dev@flink.apache.org <dev@flink.apache.org> > Subject: [EXTERNAL] Re: FW: RE: [DISCUSS] FLIP-368 Reorganize the > exceptions thrown in state interfaces > Hi David, > > Thanks for your response. > > The exceptions thrown by state interfaces are NOT retriable. For > example, there may be some elements sent to the wrong subtask due to a > non-deterministic hashCode() algorithm and the key group is not > matching. Or the rocksdb may fail to read a file if it has been > deleted by the user. If there are future implementations that are > worth retrying (such as network access), it would be better to let the > implementation itself handle the retries and provide a configuration > for this, rather than requiring users to catch these exceptions. > > Regarding the release and documentation, I have mentioned that this > change is targeted for version 1.19 with proper documentation. You may > have noticed that state interfaces are annotated with @PublicEvolving, > which means these interfaces may change across versions. The changes > are suitable for a minor release (1.18.0 currently to 1.19.0 in the > future) as defined by the API compatibility guarantees of Flink[1]. > > > > Best, > Zakelly > > > [1] > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees > > On Tue, Oct 10, 2023 at 6:19 PM David Radley <david_rad...@uk.ibm.com> > wrote: > > > > Hi, > > I notice > https://nightlies.apache.org/flink/flink-docs-master/api/java/org/apache/flink/api/common/state/ValueState.html > is an external API. I am concerned that this change will break existing > applications using the old interface, they are likely to have catches / > throws around the existing checked Exceptions. > > > > If we go with RunTimeException, I would suggest that this sort of > breaking change should be done on a Flink version change, where it is > appropriate to make breaking changes to the API with associated > documentation. > > > > If we want this change on a minor release, we could create a new class > ValueState2– that is used internally with the cleaned up Exceptions, but > still expose the old class and Exceptions for existing external > applications. I guess new applications could use the new ValueState2 . > > > > What do you think? > > Kind regards, David. > > > > > > From: David Radley <david_rad...@uk.ibm.com> > > Date: Tuesday, 10 October 2023 at 09:49 > > To: dev@flink.apache.org <dev@flink.apache.org> > > Subject: [EXTERNAL] RE: [DISCUSS] FLIP-368 Reorganize the exceptions > thrown in state interfaces > > Hi , > > The argument seems to be that the errors cannot be acted on so should be > runtime exceptions. I want to confirm that none of these errors could / > should be retriable. If there is a possibility that the state is available > at some time later then I assume a checked retriable Exception would be > appropriate for those cases; and be part of the contract with the caller. > Can we be sure that there is no possibility that the state will become > available; if so then I agree that a runtime Exception is appropriate. What > do you think? > > > > > > > > Kind regards, David. > > > > > > From: Zakelly Lan <zakelly....@gmail.com> > > Date: Monday, 9 October 2023 at 18:12 > > To: dev@flink.apache.org <dev@flink.apache.org> > > Subject: [EXTERNAL] Re: [DISCUSS] FLIP-368 Reorganize the exceptions > thrown in state interfaces > > 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. > > > > > > >> > > > > > > > > > > > > > > > > Unless otherwise stated above: > > > > IBM United Kingdom Limited > > Registered in England and Wales with number 741598 > > Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU > > > > Unless otherwise stated above: > > > > IBM United Kingdom Limited > > Registered in England and Wales with number 741598 > > Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU > > Unless otherwise stated above: > > IBM United Kingdom Limited > Registered in England and Wales with number 741598 > Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU >
