Hi Knowles, Thanks for the KIP. I may have more to say later but there's one thing I'd like to make sure to share now. In the Javadocs for the proposed SourceTask::ignoreNonRetriableProducerException method, the InvalidProducerEpochException exception class is included as an example of a non-retriable exception that may cause the new SourceTask method to be invoked. This exception should only arise if the source task's producer is a transactional producer, which is currently never the case and, once KIP-618 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-618) is merged, will only be the case when the task is running with exactly-once support. I wonder if it's safe to allow connectors to discard this exception when they're running with exactly-once support, or if the task should still be unconditionally failed in that case?
Cheers, Chris On Wed, Oct 27, 2021 at 5:39 PM John Roesler <vvcep...@apache.org> wrote: > Hi Knowles, > > Thanks for the reply! That all sounds reasonable to me, and > that's a good catch regarding the SourceRecord. > > Thanks, > -John > > On Wed, 2021-10-27 at 15:32 -0400, Knowles Atchison Jr > wrote: > > John, > > > > Thank you for the response and feedback! > > > > I originally started my first pass with the ProducerRecord<byte[], > byte[]>. > > For our connector, we need some of the information out of the > SourceRecord > > to ack our source system. If I had the actual ProducerRecord, I would > have > > to convert it back before I would be able to do anything useful with it. > I > > think there is merit in providing both records as parameters to this > > callback. Then connector writers can decide which of the representations > of > > the data is most useful to them. I also noticed that in my PR I was > sending > > the SourceRecord post transformation, when we really should be sending > the > > preTransformRecord. > > > > The Streams solution to this is very interesting. Given the nature of a > > connector, to me it makes the most sense for the api call to be part of > > that task rather than an external class that is configurable. This allows > > the connector to use state it may have at the time to inform decisions on > > what to do with these producer exceptions. > > > > I have updated the KIP and PR. > > > > Knowles > > > > On Wed, Oct 27, 2021 at 1:03 PM John Roesler <vvcep...@apache.org> > wrote: > > > > > Good morning, Knowles, > > > > > > Thanks for the KIP! > > > > > > To address your latest questions, it is fine to call for a > > > vote if a KIP doesn't generate much discussion. Either the > > > KIP was just not controversial enough for anyone to comment, > > > in which case a vote is appropriate; or no one had time to > > > review it, in which case, calling for a vote might be more > > > provacative and elicit a response. > > > > > > As far as pinging people directly, one idea would be to look > > > at the git history (git blame/praise) for the files you're > > > changing to see which committers have recently been > > > involved. Those are the folks who are most likely to have > > > valuable feedback on your proposal. It might not be > > > appropriate to directly email them, but I have seen KIP > > > discussions before that requested feedback from people by > > > name. It's probably not best to lead with that, but since no > > > one has responded so far, it might not hurt. I'm sure that > > > the reason they haven't noticed your KIP is just that they > > > are so busy it slipped their radar. They might actually > > > appreciate a more direct ping at this point. > > > > > > I'm happy to review, but as a caveat, I don't have much > > > experience with using or maintaining Connect, so caveat > > > emptor as far as my review goes. > > > > > > First of all, thanks for the well written KIP. Without much > > > context, I was able to understand the motivation and > > > proposal easily just by reading your document. > > > > > > I think your proposal is a good one. It seems like it would > > > be pretty obvious as a user what (if anything) to do with > > > the proposed method. > > > > > > For your reference, this proposal reminds me of these > > > capabilities in Streams: > > > > > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/errors/DeserializationExceptionHandler.java > > > and > > > > > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/errors/ProductionExceptionHandler.java > > > . > > > > > > I'm not sure if there's value in bringing your proposed > > > interface closer to that pattern or not. Streams and Connect > > > are quite different domains after all. At least, I wanted > > > you to be aware of them so you could consider the > > > alternative API strategy they present. > > > > > > Regardless, I do wonder if it would be helpful to also > > > include the actual ProducerRecord we tried to send, since > > > there's a non-trivial transformation that takes place to > > > convert the SourceRecord into a ProducerRecord. I'm not sure > > > what people would do with it, exactly, but it might be > > > helpful in deciding what to do about the exception, or maybe > > > even in understanding the exception. > > > > > > Those are the only thoughts that come to my mind! Thanks > > > again, > > > -John > > > > > > On Wed, 2021-10-27 at 09:16 -0400, Knowles Atchison Jr > > > wrote: > > > > Good morning, > > > > > > > > Bumping this thread. Is there someone specific on the Connect > framework > > > > team that I should ping? Is it appropriate to just call a vote? All > > > source > > > > connectors are dead in the water without a way to handle producer > write > > > > exceptions. Thank you. > > > > > > > > Knowles > > > > > > > > On Mon, Oct 18, 2021 at 8:33 AM Christopher Shannon < > > > > christopher.l.shan...@gmail.com> wrote: > > > > > > > > > I also would find this feature useful to handle errors better, does > > > anyone > > > > > have any comments or feedback? > > > > > > > > > > > > > > > On Mon, Oct 11, 2021 at 8:52 AM Knowles Atchison Jr < > > > katchiso...@gmail.com > > > > > > > > > > > wrote: > > > > > > > > > > > Good morning, > > > > > > > > > > > > Bumping this for visibility. I would like this to go into the > next > > > > > release. > > > > > > KIP freeze is Friday. > > > > > > > > > > > > Any comments and feedback are welcome. > > > > > > > > > > > > Knowles > > > > > > > > > > > > On Tue, Oct 5, 2021 at 4:24 PM Knowles Atchison Jr < > > > > > katchiso...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > Hello all, > > > > > > > > > > > > > > I would like to discuss the following KIP: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-779%3A+Allow+Source+Tasks+to+Handle+Producer+Exceptions > > > > > > > > > > > > > > The main purpose is to allow Source Tasks the ability to see > > > underlying > > > > > > > Producer Exceptions and decide what to do rather than being > > > killed. In > > > > > > our > > > > > > > use cases we would want to log/write off some information and > > > continue > > > > > > > processing. > > > > > > > > > > > > > > PR is here: > > > > > > > > > > > > > > https://github.com/apache/kafka/pull/11382 > > > > > > > > > > > > > > Any comments and feedback are welcome. > > > > > > > > > > > > > > > > > > > > > Knowles > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
