Thanks Peter for driving this FLIP. One minor comment: The code for API SinkCommitterMetricGroup should be updated as description, SinkWriterMetricGroup looks like a typo.
Best, Leonard > 2023年10月10日 下午1:21，Péter Váry <peter.vary.apa...@gmail.com> 写道： > > Hi everyone, > > It seems we have no more comments. So I would like to start a vote tomorrow > if there are no further things to discuss. > > Please let me know if you have any concerns, thanks! > > > Best, > Peter > > On Thu, Oct 5, 2023, 12:53 Gabor Somogyi <gabor.g.somo...@gmail.com> wrote: > >> Thanks for the efforts Peter! >> >> I've just analyzed it through and I think it's useful feature. >> >> +1 from my side. >> >> G >> >> >> On Thu, Oct 5, 2023 at 12:35 PM Péter Váry <peter.vary.apa...@gmail.com> >> wrote: >> >>> For the record, after the rename, the new FLIP link is: >>> >>> >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+Provide+initialization+context+for+Committer+creation+in+TwoPhaseCommittingSink >>> >>> Thanks, >>> Peter >>> >>> Péter Váry <peter.vary.apa...@gmail.com> ezt írta (időpont: 2023. okt. >> 5., >>> Cs, 11:02): >>> >>>> Thanks Gordon for the comments! >>>> >>>> 1. I have changed the FLIP name to the one proposed by you. >>>> 2. In the Iceberg sink we need access only to the Flink metrics. We >> do >>>> not specifically need the job ID in the Committer after the SinkV2 >>>> migration (more about that later). This is the reason why I have >>> stated in >>>> the FLIP, that *"We should also provide similar context information >> as >>>> we do in the Writer’s case, which should be discussed further on the >>>> mailing list."*. What I did is: I have just copied most of the >>>> org.apache.flink.api.connector.sink2.InitContext [1] properties to >> the >>>> CommitterInitContext and removed the ones which seemed excessive to >>> me. >>>> The API in the FLIP is only a draft, and I am open to any >> suggestions. >>>> >>>> In the new Iceberg Sink we need unique ids for the Committables, but we >>>> generate them in the SinkV2Aggregator [2] which is placed into the >>>> PreCommitTopology. The aggregator has access to the job ID, operator ID >>> and >>>> checkpoint ID. So no new info is needed on the Committer side there. >>>> >>>> Thanks, >>>> Peter >>>> >>>> - [1] >>>> >>> >> https://github.com/apache/flink/blob/cd95b560d0c11a64b42bf6b98107314d32a4de86/flink-core/src/main/java/org/apache/flink/api/connector/sink2/Sink.java#L95-L131 >>>> - [2] >>>> >>> >> https://github.com/apache/iceberg/pull/8653/files#diff-bfd6a564485ec60b2d53f7aa800451548d4713c5f797e76bcff95d2c8ae05ed1R77-R81 >>>> >>>> Tzu-Li (Gordon) Tai <tzuli...@apache.org> ezt írta (időpont: 2023. >> okt. >>>> 5., Cs, 8:16): >>>> >>>>> Thanks Peter for starting the FLIP. >>>>> >>>>> Overall, this seems pretty straightforward and overdue, +1. >>>>> >>>>> Two quick question / comments: >>>>> >>>>> 1. Can you rename the FLIP to something less generic? Perhaps >>> "Provide >>>>> initialization context for Committer creation in >>>>> TwoPhaseCommittingSink"? >>>>> 2. Can you describe why the job ID is needed to be exposed? >> Although >>>>> it's out of scope for this FLIP, I'm wondering if there's a need to >>> do >>>>> the >>>>> same for the sink writer InitContext. >>>>> >>>>> Thanks, >>>>> Gordon >>>>> >>>>> On Wed, Oct 4, 2023 at 11:20 AM Martijn Visser < >>> martijnvis...@apache.org> >>>>> wrote: >>>>> >>>>>> Hi all, >>>>>> >>>>>> Peter, Marton, Gordon and I had an offline sync on SinkV2 and I'm >>>>>> happy with this first FLIP on the topic. +1 >>>>>> >>>>>> Best regards, >>>>>> >>>>>> Martijn >>>>>> >>>>>> On Wed, Oct 4, 2023 at 5:48 PM Márton Balassi < >>> balassi.mar...@gmail.com >>>>>> >>>>>> wrote: >>>>>>> >>>>>>> Thanks, Peter. I agree that this is needed for Iceberg and >>> beneficial >>>>> for >>>>>>> other connectors too. >>>>>>> >>>>>>> +1 >>>>>>> >>>>>>> On Wed, Oct 4, 2023 at 3:56 PM Péter Váry < >>>>> peter.vary.apa...@gmail.com> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Team, >>>>>>>> >>>>>>>> In my previous email[1] I have described our challenges >> migrating >>>>> the >>>>>>>> existing Iceberg SinkFunction based implementation, to the new >>>>> SinkV2 >>>>>> based >>>>>>>> implementation. >>>>>>>> >>>>>>>> As a result of the discussion around that topic, I have created >>> the >>>>>> first >>>>>>>> [2] of the FLIP-s addressing the missing features there. >>>>>>>> >>>>>>>> The main goal of the FLIP-371 is to extend the currently >> existing >>>>>> Committer >>>>>>>> API by providing an initial context on Committer creation. This >>>>> context >>>>>>>> will contain - among other, less interesting data - >>>>>>>> the SinkCommitterMetricGroup which could be used to store the >>>>> generic >>>>>>>> commit related metrics, and also provide a way for the Committer >>> to >>>>>> publish >>>>>>>> its own metrics. >>>>>>>> >>>>>>>> The feature has already been requested through FLINK-25857 [3]. >>>>>>>> >>>>>>>> Please use this thread to discuss the FLIP related questions, >>>>>> proposals, >>>>>>>> feedback. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Peter >>>>>>>> >>>>>>>> - [1] >>>>> https://lists.apache.org/thread/h3kg7jcgjstpvwlhnofq093vk93ylgsn >>>>>>>> - [2] >>>>>>>> >>>>>>>> >>>>>> >>>>> >>> >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+SinkV2+Committer+imporvements >>>>>>>> - [3] https://issues.apache.org/jira/browse/FLINK-25857 >>>>>>>> >>>>>> >>>>> >>>> >>> >>
