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 > > >> > > > > > >> > > > >> > > > > > >
