Thanks Ahmed for the update, the FLIP looks good to me now. Best, Leonard
> 2024年5月22日 下午4:34，Ahmed Hamdy <hamdy10...@gmail.com> 写道： > >> >> (1) Implicitly point a public API change is not enough, Could you add a >> section Public Interfaces to enumerate all Public APIs that you proposed >> and you changed? >> It’s a standard part of a FLIP template[1]. >> > yes this is updated in the FLIP now. > > > >> (2) About the proposed public interface ResultHandler<RequestEntryT>, >> Could you explain or show how to use the methods #completeExceptionally and >> #retryForEntries? I didn’t find >> detail explanation or Usage example code to understand them. >> > > Added to the FLIP now. > > > >> (3) Could you add necessary java documents for all public API changes like >> new method AsyncSinkWriterConfiguration#setRequestTimeoutMs ? The java doc >> of [2] is a good example. >> > > sure, Added now. > > (4) Another minor reminder AsyncSinkBase is a @PublicEvolving interface >> too, please correct it, and please ensure the backward compatibility has >> been considered for all public interfaces the FLIP changed. >> > Done > > Best Regards > Ahmed Hamdy > > > On Wed, 22 May 2024 at 04:16, Leonard Xu <xbjt...@gmail.com> wrote: > >> Thanks for your reply, Ahmed. >> >>> (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. >> >> (1) Implicitly point a public API change is not enough, Could you add a >> section Public Interfaces to enumerate all Public APIs that you proposed >> and you changed? >> It’s a standard part of a FLIP template[1]. >> >> (2) About the proposed public interface ResultHandler<RequestEntryT>, >> Could you explain or show how to use the methods #completeExceptionally and >> #retryForEntries? I didn’t find >> detail explanation or Usage example code to understand them. >> >> (3) Could you add necessary java documents for all public API changes like >> new method AsyncSinkWriterConfiguration#setRequestTimeoutMs ? The java doc >> of [2] is a good example. >> >> (4) Another minor reminder AsyncSinkBase is a @PublicEvolving interface >> too, please correct it, and please ensure the backward compatibility has >> been considered for all public interfaces the FLIP changed. >> >> >> Best, >> Leonard >> [1]https://cwiki.apache.org/confluence/display/FLINK/FLIP+Template >> [2] >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-171%3A+Async+Sink >> >> >>> >>> >>> 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 >>>>>>>> >>>>>>> >>>>>> >>>> >>>> >> >>
