Hi Leonard, Thanks for joining the discussion. > （1）I’m confused about the discuss thread name ‘FLIP-451: Refactor Async > sink API’ and FLIP title/vote thread name ' > FLIP-451: Introduce timeout configuration to AsyncSink API < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-451%3A+Introduce+timeout+configuration+to+AsyncSink+API>’, > they are different for me. Could you help explain the change history? >
The discussion started with a wider scope to refactor the API for handling results and exceptions as well as adding timeout configuration, however the scope was reduced after feedback above to focus on the timeout configuration since this is the most urgent and is not tightly coupled with the remaining suggestions. (2) The FLIP-451 aims to introduce a timeout configuration, but I didn’t > find the configuration in FLIP even I lookup some historical versions of > the FLIP. Did I miss some key informations? > Yes, I tried to implicitly point that it will be added to the existing AsyncSinkWriterConfiguration to not inflate the FLIP, but I get it might be confusing. I have added the changes to the configuration classes in the FLIP to make it clearer. (3) About the code change part, there’re some un-complete pieces in > AsyncSinkWriter for example `submitRequestEntries(List<RequestEntryT> > requestEntries,);` is incorrect and `sendTime` variable I didn’t > find the place we define it and where we use it. > Thanks for catching, I fixed the incomplete methods, This should clarify how the new method is going to be integrated with the rest of the writer. Regarding the sendTime variable, I have replaced it with requestTimestamp; this is an unchanged part of the code to ensure the existing completeRequest method is unchanged. Best Regards Ahmed Hamdy On Tue, 21 May 2024 at 14:56, Leonard Xu <xbjt...@gmail.com> wrote: > Thanks Ahmed for kicking off this discussion, sorry for jumping the > discussion late. > > （1）I’m confused about the discuss thread name ‘FLIP-451: Refactor Async > sink API’ and FLIP title/vote thread name ' > FLIP-451: Introduce timeout configuration to AsyncSink API < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-451%3A+Introduce+timeout+configuration+to+AsyncSink+API>’, > they are different for me. Could you help explain the change history? > > (2) The FLIP-451 aims to introduce a timeout configuration, but I didn’t > find the configuration in FLIP even I lookup some historical versions of > the FLIP. Did I miss some key informations? > > (3) About the code change part, there’re some un-complete pieces in > AsyncSinkWriter for example `submitRequestEntries(List<RequestEntryT> > requestEntries,);` is incorrect and `sendTime` variable I didn’t > find the place we define it and where we use it. > > Sorry for jumping the discussion thread during vote phase again. > > Best, > Leonard > > > > 2024年5月21日 下午3:49，Ahmed Hamdy <hamdy10...@gmail.com> 写道： > > > > Hi Hong, > > Thanks for pointing that out, no we are not > > deprecating getFatalExceptionCons(). I have updated the FLIP > > Best Regards > > Ahmed Hamdy > > > > > > On Mon, 20 May 2024 at 15:40, Hong Liang <h...@apache.org> wrote: > > > >> Hi Ahmed, > >> Thanks for putting this together! Should we still be marking > >> getFatalExceptionCons() as @Deprecated in this FLIP, if we are not > >> providing a replacement? > >> > >> Regards, > >> Hong > >> > >> On Mon, May 13, 2024 at 7:58 PM Ahmed Hamdy <hamdy10...@gmail.com> > wrote: > >> > >>> Hi David, > >>> yes there error classification was initially left to sink implementers > to > >>> handle while we provided utilities to classify[1] and bubble up[2] > fatal > >>> exceptions to avoid retrying them. > >>> Additionally some sink implementations provide an option to short > circuit > >>> the failures by exposing a `failOnError` flag as in > >> KinesisStreamsSink[3], > >>> however this FLIP scope doesn't include any changes for retry > mechanisms. > >>> > >>> 1- > >>> > >>> > >> > https://github.com/apache/flink/blob/015867803ff0c128b1c67064c41f37ca0731ed86/flink-connectors/flink-connector-base/src/main/java/org/apache/flink/connector/base/sink/throwable/FatalExceptionClassifier.java#L32 > >>> 2- > >>> > >>> > >> > https://github.com/apache/flink/blob/015867803ff0c128b1c67064c41f37ca0731ed86/flink-connectors/flink-connector-base/src/main/java/org/apache/flink/connector/base/sink/writer/AsyncSinkWriter.java#L533 > >>> 3- > >>> > >>> > >> > https://github.com/apache/flink-connector-aws/blob/c6e0abb65a0e51b40dd218b890a111886fbf797f/flink-connector-aws/flink-connector-aws-kinesis-streams/src/main/java/org/apache/flink/connector/kinesis/sink/KinesisStreamsSinkWriter.java#L106 > >>> > >>> Best Regards > >>> Ahmed Hamdy > >>> > >>> > >>> On Mon, 13 May 2024 at 16:20, David Radley <david_rad...@uk.ibm.com> > >>> wrote: > >>> > >>>> Hi, > >>>> I wonder if the way that the async request fails could be a retriable > >> or > >>>> non-retriable error, so it would retry only for retriable (transient) > >>>> errors (like IOExceptions) . I see some talk on the internet around > >>>> retriable SQL errors. > >>>> If this was the case then we may need configuration to limit the > >> number > >>>> of retries of retriable errors. > >>>> Kind regards, David > >>>> > >>>> > >>>> From: Muhammet Orazov <mor+fl...@morazow.com.INVALID> > >>>> Date: Monday, 13 May 2024 at 10:30 > >>>> To: dev@flink.apache.org <dev@flink.apache.org> > >>>> Subject: [EXTERNAL] Re: [DISCUSS] FLIP-451: Refactor Async sink API > >>>> Great, thanks for clarifying! > >>>> > >>>> Best, > >>>> Muhammet > >>>> > >>>> > >>>> On 2024-05-06 13:40, Ahmed Hamdy wrote: > >>>>> Hi Muhammet, > >>>>> Thanks for the feedback. > >>>>> > >>>>>> Could you please add more here why it is harder? Would the > >>>>>> `completeExceptionally` > >>>>>> method be related to it? Maybe you can add usage example for it > >> also. > >>>>>> > >>>>> > >>>>> this is mainly due to the current implementation of fatal exception > >>>>> failures which depends on base `getFatalExceptionConsumer` method > >> that > >>>>> is > >>>>> decoupled from the actual called method `submitRequestEntries`, Since > >>>>> this > >>>>> is now not the primary concern of the FLIP, I have removed it from > >> the > >>>>> motivation so that the scope is defined around introducing the > >> timeout > >>>>> configuration. > >>>>> > >>>>>> Should we add a list of possible connectors that this FLIP would > >>>>>> improve? > >>>>> > >>>>> Good call, I have added under migration plan. > >>>>> > >>>>> Best Regards > >>>>> Ahmed Hamdy > >>>>> > >>>>> > >>>>> On Mon, 6 May 2024 at 08:49, Muhammet Orazov <mor+fl...@morazow.com> > >>>>> wrote: > >>>>> > >>>>>> Hey Ahmed, > >>>>>> > >>>>>> Thanks for the FLIP! +1 (non-binding) > >>>>>> > >>>>>>> Additionally the current interface for passing fatal exceptions > >> and > >>>>>>> retrying records relies on java consumers which makes it harder to > >>>>>>> understand. > >>>>>> > >>>>>> Could you please add more here why it is harder? Would the > >>>>>> `completeExceptionally` > >>>>>> method be related to it? Maybe you can add usage example for it > >> also. > >>>>>> > >>>>>>> we should proceed by adding support in all supporting connector > >>> repos. > >>>>>> > >>>>>> Should we add list of possible connectors that this FLIP would > >>>>>> improve? > >>>>>> > >>>>>> Best, > >>>>>> Muhammet > >>>>>> > >>>>>> > >>>>>> On 2024-04-29 14:08, Ahmed Hamdy wrote: > >>>>>>> Hi all, > >>>>>>> I would like to start a discussion on FLIP-451[1] > >>>>>>> The proposal comes on encountering a couple of issues while > >> working > >>>>>>> with > >>>>>>> implementers for Async Sink. > >>>>>>> The FLIP mainly proposes a new API similar to AsyncFunction and > >>>>>>> ResultFuture as well as introducing timeout handling for AsyncSink > >>>>>>> requests. > >>>>>>> The FLIP targets 1.20 with backward compatible changes and we > >> should > >>>>>>> proceed by adding support in all supporting connector repos. > >>>>>>> > >>>>>>> 1- > >>>>>>> > >>>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-451%3A+Refactor+Async+Sink+API > >>>>>>> Best Regards > >>>>>>> Ahmed Hamdy > >>>>>> > >>>> > >>>> 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 > >>>> > >>> > >> > >
