You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Sudesh Wasnik <wa...@gmail.com> on 2025/04/08 15:19:47 UTC

[DISCUSS] KIP-1158: Improve Connect SourceTask commit()

Hi all,
I would like to discuss KIP-1158.

This KIP introduces a new "commit()" method in KafkaConnect SourceTask
interface, primarily for use-cases where SourceConnectors need to
track "source-offset" commits.

Thanks in advance for reviewing !
KIP :
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=347933317

Thanks,
Sudesh

Re: [DISCUSS] KIP-1158: Improve Connect SourceTask commit()

Posted by Sudesh Wasnik <wa...@gmail.com>.

Thanks Chris ! I've updated the KIP and added a note in Javadocs, as
you suggested.

If everything looks good, should I go ahead and start a voting thread ?
Thanks again,
Sudesh

On Tue, 8 Apr 2025 at 21:47, Chris Egerton <fe...@gmail.com> wrote:

> Consistent behavior sounds good to me! We should probably add a note on
> that to the Javadocs but I don't consider this a blocker.
>
> Thanks again, no further feedback from me.
>
> On Tue, Apr 8, 2025, 12:12 Sudesh Wasnik <wa...@gmail.com> wrote:
>
> > Hello Chris ! Thanks for the quick review !
> >
> > Good point, task.commit() (existing and proposed) should fail the task on
> > exception.
> >
> > Currently, if a connector implements task.commit()
> > <
> >
> https://github.com/apache/kafka/blob/trunk/connect/api/src/main/java/org/apache/kafka/connect/source/SourceTask.java#L117-L119
> > >
> > and
> > throws an exception - this exception will be logged and ignored by
> > AbstractWorkerSourceTask
> > <
> >
> https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTask.java#L581-L587
> > >
> > and will not result in task-failure.
> > Failing the task with "commit()"'s exceptions would be a breaking change
> > (since unhandled exceptions on commit() might start failing the task
> > suddenly after upgrade).
> >
> > In this ticket, I'd like to keep the behaviour consistent with the way
> > AbstractWorkerSourceTask invokes "commit()" currently.
> >
> > However, tasks must fail if commit() throws an exception -> +1.
> > Should we deal with the breaking change in another ticket (targeting the
> > next major release) OR should we try to include the changes in this KIP
> > itself ? What would you suggest?
> >
> > Thanks,
> > Sudesh
> >
> >
> > On Tue, 8 Apr 2025 at 21:02, Chris Egerton <fe...@gmail.com>
> > wrote:
> >
> > > Hi Sudesh,
> > >
> > > Thanks for the KIP. The existing commit method is... not great. I think
> > the
> > > word "futile" you chose in the KIP is putting it lightly.
> > >
> > > The new interface looks clean, implementation seems extremely
> > > straightforward, and everything's backwards compatible.
> > >
> > > The only comment I'll leave is that we should probably document what
> the
> > > expected behavior is if a task throws from this new method. I'm
> guessing
> > > the task will fail but let me know if you have other thoughts.
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Tue, Apr 8, 2025, 11:20 Sudesh Wasnik <wa...@gmail.com> wrote:
> > >
> > > > Hi all,
> > > > I would like to discuss KIP-1158.
> > > >
> > > > This KIP introduces a new "commit()" method in KafkaConnect
> SourceTask
> > > > interface, primarily for use-cases where SourceConnectors need to
> > > > track "source-offset" commits.
> > > >
> > > > Thanks in advance for reviewing !
> > > > KIP :
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=347933317
> > > >
> > > > Thanks,
> > > > Sudesh
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-1158: Improve Connect SourceTask commit()

Posted by Chris Egerton <fe...@gmail.com>.

Consistent behavior sounds good to me! We should probably add a note on
that to the Javadocs but I don't consider this a blocker.

Thanks again, no further feedback from me.

On Tue, Apr 8, 2025, 12:12 Sudesh Wasnik <wa...@gmail.com> wrote:

> Hello Chris ! Thanks for the quick review !
>
> Good point, task.commit() (existing and proposed) should fail the task on
> exception.
>
> Currently, if a connector implements task.commit()
> <
> https://github.com/apache/kafka/blob/trunk/connect/api/src/main/java/org/apache/kafka/connect/source/SourceTask.java#L117-L119
> >
> and
> throws an exception - this exception will be logged and ignored by
> AbstractWorkerSourceTask
> <
> https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTask.java#L581-L587
> >
> and will not result in task-failure.
> Failing the task with "commit()"'s exceptions would be a breaking change
> (since unhandled exceptions on commit() might start failing the task
> suddenly after upgrade).
>
> In this ticket, I'd like to keep the behaviour consistent with the way
> AbstractWorkerSourceTask invokes "commit()" currently.
>
> However, tasks must fail if commit() throws an exception -> +1.
> Should we deal with the breaking change in another ticket (targeting the
> next major release) OR should we try to include the changes in this KIP
> itself ? What would you suggest?
>
> Thanks,
> Sudesh
>
>
> On Tue, 8 Apr 2025 at 21:02, Chris Egerton <fe...@gmail.com>
> wrote:
>
> > Hi Sudesh,
> >
> > Thanks for the KIP. The existing commit method is... not great. I think
> the
> > word "futile" you chose in the KIP is putting it lightly.
> >
> > The new interface looks clean, implementation seems extremely
> > straightforward, and everything's backwards compatible.
> >
> > The only comment I'll leave is that we should probably document what the
> > expected behavior is if a task throws from this new method. I'm guessing
> > the task will fail but let me know if you have other thoughts.
> >
> > Cheers,
> >
> > Chris
> >
> > On Tue, Apr 8, 2025, 11:20 Sudesh Wasnik <wa...@gmail.com> wrote:
> >
> > > Hi all,
> > > I would like to discuss KIP-1158.
> > >
> > > This KIP introduces a new "commit()" method in KafkaConnect SourceTask
> > > interface, primarily for use-cases where SourceConnectors need to
> > > track "source-offset" commits.
> > >
> > > Thanks in advance for reviewing !
> > > KIP :
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=347933317
> > >
> > > Thanks,
> > > Sudesh
> > >
> >
>

Re: [DISCUSS] KIP-1158: Improve Connect SourceTask commit()

Posted by Sudesh Wasnik <wa...@gmail.com>.

Hello Chris ! Thanks for the quick review !

Good point, task.commit() (existing and proposed) should fail the task on
exception.

Currently, if a connector implements task.commit()
<https://github.com/apache/kafka/blob/trunk/connect/api/src/main/java/org/apache/kafka/connect/source/SourceTask.java#L117-L119>
and
throws an exception - this exception will be logged and ignored by
AbstractWorkerSourceTask
<https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTask.java#L581-L587>
and will not result in task-failure.
Failing the task with "commit()"'s exceptions would be a breaking change
(since unhandled exceptions on commit() might start failing the task
suddenly after upgrade).

In this ticket, I'd like to keep the behaviour consistent with the way
AbstractWorkerSourceTask invokes "commit()" currently.

However, tasks must fail if commit() throws an exception -> +1.
Should we deal with the breaking change in another ticket (targeting the
next major release) OR should we try to include the changes in this KIP
itself ? What would you suggest?

Thanks,
Sudesh

On Tue, 8 Apr 2025 at 21:02, Chris Egerton <fe...@gmail.com> wrote:

> Hi Sudesh,
>
> Thanks for the KIP. The existing commit method is... not great. I think the
> word "futile" you chose in the KIP is putting it lightly.
>
> The new interface looks clean, implementation seems extremely
> straightforward, and everything's backwards compatible.
>
> The only comment I'll leave is that we should probably document what the
> expected behavior is if a task throws from this new method. I'm guessing
> the task will fail but let me know if you have other thoughts.
>
> Cheers,
>
> Chris
>
> On Tue, Apr 8, 2025, 11:20 Sudesh Wasnik <wa...@gmail.com> wrote:
>
> > Hi all,
> > I would like to discuss KIP-1158.
> >
> > This KIP introduces a new "commit()" method in KafkaConnect SourceTask
> > interface, primarily for use-cases where SourceConnectors need to
> > track "source-offset" commits.
> >
> > Thanks in advance for reviewing !
> > KIP :
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=347933317
> >
> > Thanks,
> > Sudesh
> >
>

Re: [DISCUSS] KIP-1158: Improve Connect SourceTask commit()

Posted by Chris Egerton <fe...@gmail.com>.

Hi Sudesh,

Thanks for the KIP. The existing commit method is... not great. I think the
word "futile" you chose in the KIP is putting it lightly.

The new interface looks clean, implementation seems extremely
straightforward, and everything's backwards compatible.

The only comment I'll leave is that we should probably document what the
expected behavior is if a task throws from this new method. I'm guessing
the task will fail but let me know if you have other thoughts.

Cheers,

Chris

On Tue, Apr 8, 2025, 11:20 Sudesh Wasnik <wa...@gmail.com> wrote:

> Hi all,
> I would like to discuss KIP-1158.
>
> This KIP introduces a new "commit()" method in KafkaConnect SourceTask
> interface, primarily for use-cases where SourceConnectors need to
> track "source-offset" commits.
>
> Thanks in advance for reviewing !
> KIP :
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=347933317
>
> Thanks,
> Sudesh
>