Thank you all for the feedback, the KIP has been updated. On Tue, Nov 16, 2021 at 10:46 AM Arjun Satish <arjun.sat...@gmail.com> wrote:
> One more nit: the RetryWithToleranceOperator class is not a public > interface. So we do not have to call the changes in them out in the Public > Interfaces section. > > > On Tue, Nov 16, 2021 at 10:42 AM Arjun Satish <arjun.sat...@gmail.com> > wrote: > > > Chris' point about upgrades is valid. An existing configuration will now > > have additional behavior. We should clearly call this out in the kip, and > > whenever they are prepared -- the release notes. It's a bit crummy when > > upgrading, but I do think it's better than introducing a new > configuration > > in the long term. > > > > On Mon, Nov 15, 2021 at 2:52 PM Knowles Atchison Jr < > katchiso...@gmail.com> > > wrote: > > > >> Chris, > >> > >> Thank you for the feedback. I can certainly update the KIP to state that > >> once exactly one support is in place, the task would be failed even if > >> error.tolerance were set to all. Programmatically it would still require > >> PRs to be merged to build on top of. I also liked my original > >> implementation of the hook as it gave the connector writers the most > >> flexibility in handling producer errors. I changed the original > >> implementation as the progression/changes still supported my use case > and > >> I > >> thought it would move this process along faster. > >> > >> Knowles > >> > >> On Thu, Nov 11, 2021 at 3:43 PM Chris Egerton > <chr...@confluent.io.invalid > >> > > >> wrote: > >> > >> > Hi Knowles, > >> > > >> > I think this looks good for the most part but I'd still like to see an > >> > explicit mention in the KIP (and proposed doc/Javadoc changes) that > >> states > >> > that, with exactly-once support enabled, producer exceptions that > result > >> > from failures related to exactly-once support (including but not > >> limited to > >> > ProducerFencedExcecption instances ( > >> > > >> > > >> > https://kafka.apache.org/30/javadoc/org/apache/kafka/common/errors/ProducerFencedException.html > >> > )) > >> > will not be skipped even with "errors.tolerance" set to "all", and > will > >> > instead unconditionally cause the task to fail. Your proposal that > >> > "WorkerSourceTask could check the configuration before handing off the > >> > records and exception to this function" seems great as long as we > update > >> > "handing off the records and exceptions to this function" to the > >> > newly-proposed behavior of "logging the exception and continuing to > poll > >> > the task for data". > >> > > >> > I'm also a little bit wary of updating the existing "errors.tolerance" > >> > configuration to have new behavior that users can't opt out of without > >> also > >> > opting out of the current behavior they get with "errors.tolerance" > set > >> to > >> > "all", but I think I've found a decent argument in favor of it. One > >> thought > >> > that came to mind is whether this use case was originally considered > >> when > >> > KIP-298 was being discussed. However, it appears that KAFKA-8586 ( > >> > https://issues.apache.org/jira/browse/KAFKA-8586), the fix for which > >> > caused > >> > tasks to fail on non-retriable, asynchronous producer exceptions > >> instead of > >> > logging them and continuing, was discovered over a full year after the > >> > changes for KIP-298 (https://github.com/apache/kafka/pull/5065) were > >> > merged. I suspect that the current proposal aligns nicely with the > >> original > >> > design intent of KIP-298, and that if KAFKA-8586 were discovered > before > >> or > >> > during discussion for KIP-298, non-retriable, asynchronous producer > >> > exceptions would have been included in its scope. With that in mind, > >> > although it may cause issues for some niche use cases, I think that > >> this is > >> > a valid change and would be worth the tradeoff of potentially > >> complicating > >> > life for a small number of users. I'd be interested in Arjun's > thoughts > >> on > >> > this though (as he designed and implemented KIP-298), and if this > >> analysis > >> > is agreeable, we may want to document that information in the KIP as > >> well > >> > to strengthen our case for not introducing a new configuration > property > >> and > >> > instead making this behavior tied to the existing "errors.tolerance" > >> > property with no opt-out besides using a new value for that property. > >> > > >> > My last thought is that, although it may be outside the scope of this > >> KIP, > >> > I believe your original proposal of giving tasks a hook to handle > >> > downstream exceptions is actually quite valid. The DLQ feature for > sink > >> > connectors is an extremely valuable one as it prevents data loss when > >> > "errors.tolerance" is set to "all" by allowing users to reprocess > >> > problematic records at a later date without stopping the flow of data > in > >> > their connector entirely. As others have noted, it's difficult if not > >> > outright impossible to provide a Kafka DLQ topic for source connectors > >> with > >> > the same guarantees, and so allowing source connectors the option of > >> > storing problematic records back in the system that they came from > seems > >> > like a reasonable alternative. I think we're probably past the point > of > >> > making that happen in this KIP, but I don't believe the changes you've > >> > proposed make that any harder in the future than it is now (which is > >> > great!), and I wanted to voice my general support for a mechanism like > >> this > >> > in case you or someone following along think it'd be worth it to > pursue > >> at > >> > a later date. > >> > > >> > Thanks for your KIP and thanks for your patience with the process! > >> > > >> > Cheers, > >> > > >> > Chris > >> > > >> > On Fri, Nov 5, 2021 at 8:26 AM Knowles Atchison Jr < > >> katchiso...@gmail.com> > >> > wrote: > >> > > >> > > Good morning, > >> > > > >> > > If there is no additional feedback, I am going to call a vote for > this > >> > KIP > >> > > on Monday. > >> > > > >> > > Knowles > >> > > > >> > > On Tue, Nov 2, 2021 at 10:00 AM Knowles Atchison Jr < > >> > katchiso...@gmail.com > >> > > > > >> > > wrote: > >> > > > >> > > > Third time's the charm. > >> > > > > >> > > > I've added a getter for the RetryWithToleranceOperator to get the > >> > > > ToleranceType. I've updated WorkerSourceTask to check this setting > >> to > >> > see > >> > > > if it is ToleranceType.ALL. > >> > > > > >> > > > Setting "errors.tolerance" to "all" solves both problems: > >> > > > > >> > > > 1. Use an existing configuration > >> > > > 2. Moves the configuration back to the connector/task level > instead > >> of > >> > at > >> > > > the connect worker level. > >> > > > > >> > > > I've updated the KIP and PR. > >> > > > > >> > > > Additional thoughts and feedback are welcome. > >> > > > > >> > > > Knowles > >> > > > > >> > > > On Mon, Nov 1, 2021 at 2:00 AM Arjun Satish < > arjun.sat...@gmail.com > >> > > >> > > > wrote: > >> > > > > >> > > >> Looks really nice. Thanks for the changes. Couple of suggestions: > >> > > >> > >> > > >> 1. Can we reuse any of the existing configs, instead of > >> introducing a > >> > > new > >> > > >> one? I’m wondering if the error.tolerance configuration’s scope > >> can be > >> > > >> increased to include produce errors as well. That’ll help us keep > >> > number > >> > > >> of > >> > > >> configs in check. Effectively, if error.tolerance is set to all, > >> then > >> > > the > >> > > >> behavior would be like how you describe the worker would ignore > >> > producer > >> > > >> errors. > >> > > >> > >> > > >> 2. If we do choose to have a new config, could you please call > out > >> the > >> > > >> possible values it can take in the kip? > >> > > >> > >> > > >> Thanks again! > >> > > >> > >> > > >> Best, > >> > > >> > >> > > >> > >> > > >> On Fri, Oct 29, 2021 at 9:53 AM Knowles Atchison Jr < > >> > > >> katchiso...@gmail.com> > >> > > >> wrote: > >> > > >> > >> > > >> > Arjun, > >> > > >> > > >> > > >> > Thank you for your feedback, I have updated the KIP. > >> > > >> > > >> > > >> > This solution is more elegant than my original proposal; > however, > >> > > after > >> > > >> > working on the implementation, we have now pushed the > >> configuration > >> > > from > >> > > >> > the connector/task itself back to the connect worker. All tasks > >> > > running > >> > > >> on > >> > > >> > the worker would share this ignore producer exception > >> configuration > >> > > >> flag. > >> > > >> > This works for my use cases where I cannot envision setting > this > >> for > >> > > >> only > >> > > >> > one type of connector we have, but this does take the choice > out > >> of > >> > > the > >> > > >> > hands of the connector developer. I suppose that is for the > best, > >> > in a > >> > > >> > vacuum only the worker should have a say in how it handles > >> message > >> > > >> > production. > >> > > >> > > >> > > >> > Additional thoughts and feedback are welcome. > >> > > >> > > >> > > >> > Knowles > >> > > >> > > >> > > >> > On Thu, Oct 28, 2021 at 10:54 AM Arjun Satish < > >> > arjun.sat...@gmail.com > >> > > > > >> > > >> > wrote: > >> > > >> > > >> > > >> > > Yes, that makes sense. And it fits in very nicely with the > >> current > >> > > >> error > >> > > >> > > handling framework. > >> > > >> > > > >> > > >> > > On Thu, Oct 28, 2021 at 10:39 AM Knowles Atchison Jr < > >> > > >> > > katchiso...@gmail.com> > >> > > >> > > wrote: > >> > > >> > > > >> > > >> > > > That would work. I originally thought that it would be > >> confusing > >> > > to > >> > > >> > > > overload that function when a Record that wasn't actually > >> > written, > >> > > >> but > >> > > >> > > > looking at SourceTask more closely, in > >> > commitRecord(SourceRecord, > >> > > >> > > > RecordMetadata), the RecordMetadata is set to null in the > >> event > >> > > of a > >> > > >> > > > filtered transformation so the framework is already doing > >> this > >> > in > >> > > a > >> > > >> > > certain > >> > > >> > > > regard. > >> > > >> > > > > >> > > >> > > > Knowles > >> > > >> > > > > >> > > >> > > > On Thu, Oct 28, 2021 at 10:29 AM Arjun Satish < > >> > > >> arjun.sat...@gmail.com> > >> > > >> > > > wrote: > >> > > >> > > > > >> > > >> > > > > To ack the message back to the source system, we already > >> have > >> > a > >> > > >> > > > > commitRecord method. Once the bad record is handled by > >> > skip/dlq, > >> > > >> we > >> > > >> > > could > >> > > >> > > > > just call commitRecord() on it? > >> > > >> > > > > > >> > > >> > > > > On Thu, Oct 28, 2021 at 9:35 AM Knowles Atchison Jr < > >> > > >> > > > katchiso...@gmail.com > >> > > >> > > > > > > >> > > >> > > > > wrote: > >> > > >> > > > > > >> > > >> > > > > > Hi Chris, > >> > > >> > > > > > > >> > > >> > > > > > Thank you for your reply! > >> > > >> > > > > > > >> > > >> > > > > > It is a clarity error regarding the javadoc. I am not > >> > > >> operationally > >> > > >> > > > > > familiar with all of the exceptions Kafka considers > >> > > >> non-retriable, > >> > > >> > > so I > >> > > >> > > > > > pulled the list from Callback.java: > >> > > >> > > > > > > >> > > >> > > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > >> > > >> > > >> > >> > > > >> > > >> > https://github.com/apache/kafka/blob/1afe2a5190e9c98e38c84dc793f4303ea51bc19b/clients/src/main/java/org/apache/kafka/clients/producer/Callback.java#L35 > >> > > >> > > > > > to be an illustrative example of the types of > exceptions > >> > that > >> > > >> would > >> > > >> > > > kill > >> > > >> > > > > > the connector outright. Any exception thrown during the > >> > > producer > >> > > >> > > write > >> > > >> > > > > will > >> > > >> > > > > > be passed to this handler. I will update the KIP/PR to > be > >> > more > >> > > >> > clear > >> > > >> > > on > >> > > >> > > > > > this matter. > >> > > >> > > > > > > >> > > >> > > > > > You raise an excellent point, how should the framework > >> > protect > >> > > >> the > >> > > >> > > > > > connector or developer from themselves? If a connector > >> > enables > >> > > >> > > > > exactly-once > >> > > >> > > > > > semantics, it would make sense to me to have the task > >> > killed. > >> > > >> The > >> > > >> > > > > framework > >> > > >> > > > > > should enforce this type of misconfiguration that would > >> > break > >> > > >> the > >> > > >> > > > > internal > >> > > >> > > > > > semantics of KIP-618. WorkerSourceTask could check the > >> > > >> > configuration > >> > > >> > > > > before > >> > > >> > > > > > handing off the records and exception to this function, > >> fail > >> > > >> > initial > >> > > >> > > > > > configuration check, or something of that nature. > >> > > >> > > > > > > >> > > >> > > > > > Hi Arjun, > >> > > >> > > > > > > >> > > >> > > > > > Thank you for your response! > >> > > >> > > > > > > >> > > >> > > > > > My specific use case is our custom JMS connector. We > ack > >> > back > >> > > to > >> > > >> > the > >> > > >> > > > jms > >> > > >> > > > > > broker once Kafka commits the record. We thread out our > >> JMS > >> > > >> > consumer > >> > > >> > > > such > >> > > >> > > > > > that I would need access to the SourceRecord to confirm > >> we > >> > are > >> > > >> > going > >> > > >> > > to > >> > > >> > > > > > throw away the message. > >> > > >> > > > > > > >> > > >> > > > > > Skipping such records, writing some log messages, > and/or > >> > > writing > >> > > >> > some > >> > > >> > > > > error > >> > > >> > > > > > context to a DLQ would cover most if not all of the use > >> > cases > >> > > I > >> > > >> > > > envision. > >> > > >> > > > > > > >> > > >> > > > > > "discard.message.on.producer.exception": "true" > >> > > >> > > > > > > >> > > >> > > > > > or some equivalent would get my personal use case 99% > of > >> the > >> > > way > >> > > >> > > > there. I > >> > > >> > > > > > would still need some kind of callback from inside the > >> > > connector > >> > > >> > with > >> > > >> > > > the > >> > > >> > > > > > Source Record to successfully ack back to my source > >> system. > >> > > >> > > > > > > >> > > >> > > > > > I have updated the KIP regarding the callback being > >> executed > >> > > in > >> > > >> a > >> > > >> > > > > different > >> > > >> > > > > > thread than poll(). > >> > > >> > > > > > > >> > > >> > > > > > Knowles > >> > > >> > > > > > > >> > > >> > > > > > On Thu, Oct 28, 2021 at 2:02 AM Arjun Satish < > >> > > >> > arjun.sat...@gmail.com > >> > > >> > > > > >> > > >> > > > > > wrote: > >> > > >> > > > > > > >> > > >> > > > > > > Hi Knowles, > >> > > >> > > > > > > > >> > > >> > > > > > > Thanks for the KIP! > >> > > >> > > > > > > > >> > > >> > > > > > > Could you please call out some use-cases on what the > >> > source > >> > > >> > > > connectors > >> > > >> > > > > > > would do when they hit such exceptions? I'm wondering > >> if > >> > we > >> > > >> would > >> > > >> > > > need > >> > > >> > > > > to > >> > > >> > > > > > > do anything other than skipping such records, writing > >> some > >> > > log > >> > > >> > > > > messages, > >> > > >> > > > > > > and/or writing some error context to a DLQ? > >> > > >> > > > > > > > >> > > >> > > > > > > One of the goals for Connect was to abstract away > >> > > intricacies > >> > > >> of > >> > > >> > > > Kafka > >> > > >> > > > > > > topics, clients etc, so that connectors could focus > on > >> the > >> > > >> > external > >> > > >> > > > > > systems > >> > > >> > > > > > > themselves. Ideally, we'd want to see if we could > call > >> out > >> > > the > >> > > >> > most > >> > > >> > > > > > common > >> > > >> > > > > > > cases and handle them in the framework itself, > instead > >> of > >> > > >> > > delegating > >> > > >> > > > > them > >> > > >> > > > > > > back to the connector. This way, instead of the new > >> API, > >> > > we'd > >> > > >> > > > probably > >> > > >> > > > > > > introduce some more configuration options, but they > >> could > >> > be > >> > > >> > > > applicable > >> > > >> > > > > > to > >> > > >> > > > > > > all the connectors that are out there. > >> > > >> > > > > > > > >> > > >> > > > > > > Also, If the above mentioned are the most common > uses, > >> > then > >> > > we > >> > > >> > > could > >> > > >> > > > > > apply > >> > > >> > > > > > > KIP-298 (with some adjustments) to source connectors > >> for > >> > > >> > > > non-retriable > >> > > >> > > > > > > producer errors. > >> > > >> > > > > > > > >> > > >> > > > > > > If we decide to go with the API you are referring to > >> > though, > >> > > >> > would > >> > > >> > > > the > >> > > >> > > > > > > preTransformation record suffice? SMTs can be causing > >> the > >> > > >> actual > >> > > >> > > > issues > >> > > >> > > > > > > (for example, changing the topic name) that cause > these > >> > > >> > > non-retriable > >> > > >> > > > > > > exceptions. The new callback might be receiving > >> > insufficient > >> > > >> > > context > >> > > >> > > > to > >> > > >> > > > > > do > >> > > >> > > > > > > any corrective action. > >> > > >> > > > > > > > >> > > >> > > > > > > In the documentation for the new API, we might want > to > >> > > specify > >> > > >> > that > >> > > >> > > > > this > >> > > >> > > > > > > callback will be called from a different thread than > >> the > >> > > ones > >> > > >> > > calling > >> > > >> > > > > > > poll(). So any shared objects must be protected > >> > > appropriately. > >> > > >> > > > > > > > >> > > >> > > > > > > Cheers, > >> > > >> > > > > > > > >> > > >> > > > > > > On Wed, Oct 27, 2021 at 7:01 PM Chris Egerton > >> > > >> > > > > > <chr...@confluent.io.invalid > >> > > >> > > > > > > > > >> > > >> > > > > > > wrote: > >> > > >> > > > > > > > >> > > >> > > > > > > > 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 > >> > > >> > > > > > > > > > > > > > > > >> > > >> > > > > > > > > > > > > > > >> > > >> > > > > > > > > > > > > > >> > > >> > > > > > > > > > > > >> > > >> > > > > > > > > > > > >> > > >> > > > > > > > > > > > >> > > >> > > > > > > > > > >> > > >> > > > > > > > > > >> > > >> > > > > > > > > > >> > > >> > > > > > > > > >> > > >> > > > > > > > >> > > >> > > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > >> > > >> > > >> > >> > > > > >> > > > >> > > >> > > >
