Hello all,
I'd actually like to retract the earlier additions to the KIP and point out
that since I've started the voting process and gotten some responses, I
will not be making any major changes to the KIP as it would require a
re-voting process.
Thanks,
Aakash
On Tue, May 19, 2020 at 2:31 PM A
Hi Chris and others,
Yes, you are correct; I looked through KIP-298 to understand it better. I
agree with your idea to handle "errors.tolerance=none."
I see, you are basically saying you are in favor of standardizing handling
what to set the reporter to if it is not configured. I am on board with
Hi Aakash,
> If "errors.tolerance=none", should it not be the case that the error
reporter does not even report any error; rather, the task just fails after
throwing the error? I do understand the point you are saying about
duplicates, though.
I believe the "errors.tolerance" property dictates wh
Hi Arjun,
I am not very familiar with how the potential heartbeat failure would cause
more failures when consuming subsequent records. Can you elaborate on this?
Thanks,
Aakash
On Tue, May 19, 2020 at 10:03 AM Arjun Satish
wrote:
> One more concern with the connector blocking on the Future's g
Hi Chris,
Thanks for the suggestions.
If "errors.tolerance=none", should it not be the case that the error
reporter does not even report any error; rather, the task just fails after
throwing the error? I do understand the point you are saying about
duplicates, though.
You raise a good point abou
Hi Randall,
First off, thank you for the incredibly detailed example. I don't mind
walls of text. I found it very helpful. I especially liked the idea about
modifying how the framework invokes "SinkTask::preCommit" to take most of
the work out of developers' hands in the common case of a "fire-and
One more concern with the connector blocking on the Future's get() is that
it may cause the task's consumer to fail to heartbeat (since there is no
independent thread to do this). That would then cause failures when we
eventually try to consume more records after returning from put(). The
developer
Can we get a couple of examples that shows utility of waiting on the
Future<>? Also, in preCommit() we would report back on the incomplete
offsets. So that feedback mechanism will already exists for developers who
want to manually manage this.
On Tue, May 19, 2020 at 8:03 AM Randall Hauch wrote:
Thanks, Aakash, for updating the KIP.
On Tue, May 19, 2020 at 2:18 AM Arjun Satish wrote:
> Hi Randall,
>
> Thanks for the explanation! Excellent point about guaranteeing offsets in
> the async case.
>
> If we can guarantee that the offsets will be advanced only after the bad
> records are repor
Hi,
Please, tell me how to unsubscribe.
Thanks,
Serhii
On Mon, 18 May 2020, 23:44 Aakash Shah, wrote:
> Hi Chris,
>
> I agree with your point.
>
> Randall, Konstantine, do you guys mind weighing in on any benefit of adding
> asynchronous functionality using a Future in the KIP right now? It seem
Hi Randall,
Thanks for the explanation! Excellent point about guaranteeing offsets in
the async case.
If we can guarantee that the offsets will be advanced only after the bad
records are reported, then is there any value is the Future<> return type?
I feel we can declare the function with a void
Hi Randall,
I really appreciate the highly detailed explanation. It clears up the
advantages of an asynchronous design using Futures, specifically because
get() does not necessarily need to be called due to the guarantee put in
place by the framework that you mentioned. I think that if this guaran
Thanks for the detailed explanation Randall.
I think it highlights nicely how the common practice of overlapping
communication with computation (or other communication) in concurrent
systems can be useful and practical in this case.
I also agree with the amendment around `preCommit` and the guara
Hi, Chris, Aakash, and others:
First of all, apologies for the extremely long email. Secondly, thanks for
the input on this KIP. The timing is unfortunately, but I do believe we're
agreed on most points.
Chris asked earlier:
> I'm still unclear on how futures are going to provide any benefit to
Hi Chris,
I agree with your point.
Randall, Konstantine, do you guys mind weighing in on any benefit of adding
asynchronous functionality using a Future in the KIP right now? It seems to
me that it only provides user control on when the thread will be blocked,
and if we are going to process all t
Hi Aakash,
Yep, that's pretty much it. I'd also like to emphasize that we should be
identifying practical use cases for whatever API we provide. Giving
developers a future that can be made synchronous with little effort seems
flexible, but if that's all that developers are going to do with it anyw
Hi all,
Chris, I see your points about whether Futures provide much benefit at all
as they are not truly fully asynchronous.
Correct me if I am wrong, but I think what you are trying to point out is
that if we have the option to add additional functionality later (in a
simpler way too since we ar
Hi Aakash,
I asked this earlier about whether futures were the right way to go, if we
wanted to enable asynchronous behavior at all:
> I'm still unclear on how futures are going to provide any benefit to
developers, though. Blocking on the return of such a future slightly later
on in the process
Hi Arjun,
Thanks for your feedback.
I agree with moving to Future, those are good points.
I believe an earlier point made for asynchronous functionality were that
modern APIs tend to be asynchronous as they result in more expressive and
better defined APIs.
Additionally, because a lot of Kafka C
My apologies, had a typo. Meant to say "I will now open up a vote."
Thanks,
Aakash
On Sun, May 17, 2020 at 4:55 PM Aakash Shah wrote:
> Hi all,
>
> Thanks for all the feedback thus far. I've updated the KIP with all the
> suggestions. I will not open up a vote.
>
> Thanks,
> Aakash
>
> On Sun,
Hi all,
Thanks for all the feedback thus far. I've updated the KIP with all the
suggestions. I will not open up a vote.
Thanks,
Aakash
On Sun, May 17, 2020 at 3:45 PM Randall Hauch wrote:
> All good points regarding `Future` instead of
> `Future`, so +1 to that change.
>
> A few more nits. The
All good points regarding `Future` instead of
`Future`, so +1 to that change.
A few more nits. The following sentences should be removed because they
actually describe a change from the current DLQ functionality that already
sets `max.in.flight.requests.per.connection=1` by default:
"In order to
Thanks for all the feedback, folks.
re: having a callback as a parameter, I agree that at this point, it might
not add much value to the proposal.
re: synchronous vs asynchronous, is the motivation performance/higher
throughput? Taking a step back, calling report(..) in the new interface
does a c
Hi Konstantine,
Given that the reporter interface is intentionally agnostic about how
records are handled and does not necessarily entail writes to a DLQ, I'm
also in favor of not specifying a return type for the reporting mechanism.
I'm still unclear on how futures are going to provide any benef
Hi all,
I've updated the KIP to reflect all the new agreed-upon suggestions.
Please let me know if you have any more suggestions.
Thanks,
Aakash
On Sun, May 17, 2020 at 12:06 PM Konstantine Karantasis <
konstant...@confluent.io> wrote:
> Hi all,
>
> I'm on board with adding an interface in the
Hi all,
I'm on board with adding an interface in the Connect API as Arjun
suggested. Slightly higher commitment and maintenance but it also gives us
an easier path to future extensions in this scope (error handling). The
usage is equivalent to adding just a new method with known types to
`SinkTask
If that's the case, I think framework should not commit if there are any
outstanding records in teh reporter. That would prevent the scenario where
we could potentially lose records frm being sent either to Sink/the
reporter. WDYT about the KIP including that as part of the design?
On Sun, May 17,
On Sun, May 17, 2020 at 12:42 PM Magesh kumar Nandakumar <
mage...@confluent.io> wrote:
> Randall
>
> Thanks a lot for your thoughts. I was wondering if we would ever have to
> make the API asynchronous, we could expose it as a new method right? If
> that's a possibility would it be better if the
Thanks, Aakash.
After thinking about my previous proposal, I would like to retract the
suggestion of adding the `report(SinkTask, Throwable, Callable)` method
from the new interface for the following reasons:
1. The `Callable` interface requires the sink task developer to handle
an error be
Randall
Thanks a lot for your thoughts. I was wondering if we would ever have to
make the API asynchronous, we could expose it as a new method right? If
that's a possibility would it be better if the API explicitly has semantics
of a synchronous API if the implementation is indeed going to be
sync
Hi Randall,
Thanks for the suggestions. Now that we are adding an interface, I think it
is a good idea to overload the report method to support both cases.
> I guess it boils down to this question: do we know today that we will
> *never* want the reporter to write asynchronously?
Originally, I b
On Sun, May 17, 2020 at 11:01 AM Magesh kumar Nandakumar <
mage...@confluent.io> wrote:
> Thanks Randall. The suggestion i made also has a problem when reporter
> isn't enabled where it could potentially write records after error records
> to sink before failing.
>
> The other concern i had with r
Thanks, Arjun! This has been very helpful.
Looking in your POC and thinking in terms of the current KIP, it sounds
like the suggestion is to keep the same method signature for reporting
errors, but to change from the `BiFunction` to a new `ErrantRecordReporter` interface. More concretely,
I'd sugg
Thanks Randall. The suggestion i made also has a problem when reporter
isn't enabled where it could potentially write records after error records
to sink before failing.
The other concern i had with reporter being asynchronous. For some reason
if the reporter is taking longer because of say a spec
Magesh, we have talked above overloading various existing SinkTask methods,
and we concluded that this style of evolution complicates migration,
whereas providing the reporter via the context follows existing patterns in
the API and simplifies backward compatibility concerns. Arjun's research
shows
Have we considered returning error records by overriding flush/precommit?
If we think aesthetics is important this on my opinion is one possible
abstractions that could be cleaner. This would also mean that connector
developers wouldn't have to worry about a new reporter or think if its
synchronous
Hi Arjun,
This is great news. Given that we're already willing to ask developers to
catch a method-not-found exception and it sounds like a new interface can
be handled in a similar try/catch block in a same place, I like the idea of
a new fleshed-out interface instead of a BiConsumer or a BiFunct
ok folks, this is my POC PR:
https://github.com/wicknicks/kafka/tree/kip-610-2.4.1. connectors built
from this were copied into a fresh installation of Kafka Connect (v2.5, the
latest version), and ran. Without proper try-catch, the tasks would fail.
But when the appropriate exceptions were handled
Thanks for updating the KIP, Aakash. A few comments on the updated content
there:
In order to avoid error records being written out of order (for example,
> due to retries), the developer can use
> `max.in.flight.requests.per.connection=1` in their implementation for
> writing error records.
>
IM
Yeah I had tried this locally on java 8 and 11, and it had seemed to work.
Let me clean up and publish my code in a branch somewhere so we can take a
look at it.
Thanks,
On Sat, May 16, 2020 at 3:39 PM Randall Hauch wrote:
> Have you tried this? IIUC the problem is with the new type, and any cl
Have you tried this? IIUC the problem is with the new type, and any class
that uses ‘ErrantRecordReporter’ with an import would fail to be loaded by
the classloader if the type does not exist (I.e., pre-2.9 Connect
runtimes). Catching that ClassNotFoundException and dynamically importing
the type i
Hi Arjun,
Thanks for this suggestion. I actually like this a lot because a defined
interface looks more appealing and is clearer in its intention. Since we
are still using NoSuchMethodException to account for backwards
compatibility, this works for me. I can't see any drawbacks besides having
to c
Thanks Konstantine, happy to write something up in a KIP. But I think it
would be redundant if we add this kip. What do you think?
Also, Randall, yes that API would work. But, if we expect the developers to
catch NoSuchMethodErrors, then should we also go ahead and make a class
that would have a r
Hi Randall,
Thanks for the suggestion. I've updated the KIP with the agreed upon
changes as well as the new suggestions Randall mentioned:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-610%3A+Error+Reporting+in+Sink+Connectors
Please let me know what you think.
Thanks,
Aakash
On Sat, M
Thanks for following up Randall.
I agree with your latest suggestion. It was good that we explored several
options but accessing the context to obtain the reporter in Kafka Connect
versions that support this feature makes the most sense. The burden for
connector developers that want to use this re
Thanks again for the active discussion!
Regarding the future-vs-callback discussion: I did like where Chris was
going with the Callback, but he raises good point that it's unclear what to
use for the reporter type, since we'd need three parameters. Introducing a
new interface makes it much harder
Thanks for the quick response Aakash.
With respect to deprecation, this refers to deprecating this method in
newer versions of Kafka Connect (and eventually removing it).
As a connector developer, if you want your connector to run across a wide
spectrum of Connect versions, you'll have to take th
Hi Konstantine,
I don't believe a callback-based implementation would require additional
threads to be spun up. The callback could be invoked by the framework
whenever the work for reporting a record is done, on that same thread or,
in the case of a DLQ topic reporter, in a callback from the produ
+1
On Sat, May 16, 2020 at 9:55 AM Konstantine Karantasis <
konstant...@confluent.io> wrote:
> Hi Arjun,
>
> I think I agree with you that subject is interesting. Yet, I feel it
> belongs to a separate future KIP. Reading the proposal in the KIP format
> will help, at least myself, to understand
Hi Konstantine,
Thanks a lot for your feedback.
These are all good points, especially that we already have the threads we
need and that we'd rather not spin up additional. It is also true we should
consider the level of control we want to provide to the developer rather
than overstating the burde
Hi Arjun,
I think I agree with you that subject is interesting. Yet, I feel it
belongs to a separate future KIP. Reading the proposal in the KIP format
will help, at least myself, to understand it better.
Having said that, for the purpose of simplifying error handling for sink
tasks, the discussi
> I believe it is important to relieve as much of the burden of
> implementation as possible from the developer in this case, and thus I
> think using a Callback rather than a Future would be easier on the
> developer, while adding asynchronous functionality with the ability to
> opt-in synchronous
I'm kinda hoping that we get to an approach on how to extend the Connect
framework. Adding parameters in the put method is nice, and maybe works for
now, but I'm not sure how scalable it is. It'd great to be able to add more
functionality in the future. Couple of examples:
- make the metrics regis
Just wanted to clarify that I am on board with adding the overloaded
put(...) method.
Thanks,
Aakash
On Fri, May 15, 2020 at 7:00 PM Aakash Shah wrote:
> Hi Randall and Konstantine,
>
> As Chris and Arjun mentioned, I think the main concern is the potential
> gap in which developers don't imple
Hi Randall and Konstantine,
As Chris and Arjun mentioned, I think the main concern is the potential gap
in which developers don't implement the deprecated method due to a
misunderstanding of use cases. Using the setter method approach ensures
that the developer won't break backwards compatibility
Hi Randall,
I think the problem here is that users aren't likely to implement a
deprecated method, and may even remove that method from their connector
after seeing that it is now deprecated. This would render that connector
incompatible with older runtimes. Why deprecate a method that we don't pl
On Fri, May 15, 2020 at 3:13 PM Arjun Satish wrote:
> Couple of thoughts:
>
> 1. If we add new parameters to put(..), and new connectors implement only
> this method, it makes them backward incompatible with older workers. I
> think newer connectors may only choose to only implement the latest me
Hi all,
I think if we're going to avoid adding methods to the SinkTaskContext due
to concerns over compatibility, we might also want to ensure that the
approach we follow instead doesn't have similar pitfalls. An overloaded
"put", "initialize", or "start" isn't quite as troublesome with regards to
Couple of thoughts:
1. If we add new parameters to put(..), and new connectors implement only
this method, it makes them backward incompatible with older workers. I
think newer connectors may only choose to only implement the latest method,
and we are passing the compatibility problems back to the
Hi Konstantine,
Thanks for the insight. In that case, I will make the appropriate changes
to the KIP to support asynchronous functionality with a BiFunction.
Best,
Aakash
On Fri, May 15, 2020 at 11:45 AM Konstantine Karantasis <
konstant...@confluent.io> wrote:
> Thanks for the quick response A
On Fri, May 15, 2020 at 1:45 PM Konstantine Karantasis <
konstant...@confluent.io> wrote:
> Thanks for the quick response Aakash.
>
> To your last point, modern APIs like this tend to be asynchronous (see
> admin, producer in Kafka) and such definition results in more expressive
> and well defined
Thanks for the quick response Aakash.
To your last point, modern APIs like this tend to be asynchronous (see
admin, producer in Kafka) and such definition results in more expressive
and well defined APIs.
What you describe is easily an opt-in feature for the connector developer.
At the same time,
Thanks for the additional feedback.
I see the benefits of adding an overloaded put(...) over alternatives and I
am on board going forward with this approach. It will definitely set forth
a contract of where the reporter will be used with better aesthetics.
The original idea of going with a synchr
Small correction. I didn't mean to declare the new method `abstract`.
I agree with Randall's suggestion to give it a default implementation that
will call the old `put` and at the same time deprecate the old `put`.
Konstantine
On Fri, May 15, 2020 at 10:19 AM Konstantine Karantasis <
konstant...@
I was on the fence between the various overloading methods myself, liking
`start(...)` the least.
Initially, I thought we were interested in offering the ability to call the
reporter out of band, outside `put`.
But after your replies I understand you don't think that's the case, and I
also agree t
Konstantine said:
> I notice Randall also used BiFunction in his example, I wonder if it's for
> similar reasons.
>
Nope. Just a typo on my part.
There appear to be three outstanding questions.
First, Konstantine suggested calling this "failedRecordReporter". I think
this is minor, but using th
Hi,
Randall's suggestion is really good. I think it gives the flexibility required
and also
keeps the interface the right way round.
Thanks,
Andrew Schofield
On 15/05/2020, 02:07, "Aakash Shah" wrote:
> Hi Randall,
>
> Thanks for the feedback.
>
> 1. This is a great suggestion, but I find t
Thanks for KIP Aakash.
This proposal will address a significant gap in error handling for sink
connectors, so I'd also like to see it implemented.
Interesting discussion so far and I agree with a lot of what's been said.
First of all I agree with Andrew. When I first read the KIP I also felt
this
Hi Randall,
Thanks for the feedback.
1. This is a great suggestion, but I find that adding an overloaded
put(...) which essentially deprecates the old put(...) to only be used when
a connector is deployed on older versions of Connect adds enough of a
complication that could cause connectors to br
Hi Andrew,
Thanks for your comments.
Based on my understanding from
https://kafka.apache.org/25/javadoc/org/apache/kafka/connect/sink/SinkTask.html,
put(...) can be asynchronous but not necessarily should be, specifically
in: "As records are fetched from Kafka, they will be passed to the sink
tas
Hi Aakash,
Responding to your question from a separate KIP-610 discussion thread here,
with my comments:
> 4. Great point about the addition of the extra configuration properties.
By
"If we decide to include these properties, we should also update the
"Synchrony" section to be agnostic about what
Hi Randall,
I think I prefer the method originally specified in the KIP. A separate
method can come with a contract about if/when it's called so that tasks can
assume that it's only invoked once over their lifetime, and allows
connector developers to separate the logic for storing (and possibly do
Hi, Aakash.
Thanks for the KIP. Connect does need an improved ability for sink
connectors to report individual records as being problematic, and this
integrates nicely with the existing DLQ feature.
I also appreciate the desire to maintain compatibility so that connectors
can take advantage of th
Hi Aakash,
Thanks for sorting out the replies to the mailing list.
First, I do like the idea of improving error reporting in sink connectors. I'd
like a simple
way to put bad records onto the DLQ.
I think this KIP is considerably more complicated than it seems. The guidance
on the
SinkTask.put(
Hi Chris,
Thanks for the feedback!
1. Great point, this is the more correct general aim of the proposal.
2. Thanks for the suggestions on points a and b, they are both great. I
will incorporate them.
3. Yep, I'll add this to the sample code and add an explanation.
4. Great point about the addi
I wasn't previously added to the dev mailing list, so I'd like to post my
discussion with Andrew Schofield below for visibility and further
discussion:
Hi Andrew,
Thanks for the reply. The main concern with this approach would be its
backward compatibility. I’ve highlighted the thoughts around th
Hi Aakash,
Thanks for the KIP! Given the myriad of different error-handling mechanisms
available in sink connectors today, I think it'd be a great improvement to
add this kind of support to the framework and not only take some of the
development burden off of connector writers but also some of the
Hi,
Thanks for the KIP.
I wonder whether this idea would be better implemented using a new method on the
SinkTaskContext.
public void putFailed(Collection records)
Then the rest of KIP-298 could apply. Failed records could be put to the DLQ or
logged, as
appropriate. I think there's real valu
78 matches
Mail list logo
