You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Péter Váry <pe...@gmail.com> on 2023/10/04 13:55:49 UTC
[DISCUSS] FLIP-371: SinkV2 Committer imporvements
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
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Péter Váry <pe...@gmail.com>.
Thanks Leonard for catching!
Fixed
Leonard Xu <xb...@gmail.com> ezt írta (időpont: 2023. okt. 16., H, 8:38):
> 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 <pe...@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 <ga...@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 <pe...@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 <pe...@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 <tz...@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 <
>
> martijnvisser@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.marton@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.apache@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
>
>
>
>
>
>
>
>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Leonard Xu <xb...@gmail.com>.
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 <pe...@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 <ga...@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 <pe...@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 <pe...@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 <tz...@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 <
>>> martijnvisser@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.marton@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.apache@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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Péter Váry <pe...@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 <ga...@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 <pe...@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 <pe...@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 <tz...@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 <
> > martijnvisser@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.marton@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.apache@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
> > >> > > >
> > >> >
> > >>
> > >
> >
>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Gabor Somogyi <ga...@gmail.com>.
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 <pe...@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 <pe...@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 <tz...@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 <
> martijnvisser@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.marton@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.apache@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
> >> > > >
> >> >
> >>
> >
>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Péter Váry <pe...@gmail.com>.
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 <pe...@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 <tz...@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 <ma...@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.marton@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.apache@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
>> > > >
>> >
>>
>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Péter Váry <pe...@gmail.com>.
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 <tz...@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 <ma...@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 <ba...@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.apache@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
> > > >
> >
>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by "Tzu-Li (Gordon) Tai" <tz...@apache.org>.
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 <ma...@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 <ba...@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 <pe...@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
> > >
>
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Martijn Visser <ma...@apache.org>.
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 <ba...@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 <pe...@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
> >
Re: [DISCUSS] FLIP-371: SinkV2 Committer imporvements
Posted by Márton Balassi <ba...@gmail.com>.
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 <pe...@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
>