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 <wasnik...@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 <fearthecel...@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 <wasnik...@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 > > > > > >
