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