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