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 <as...@confluent.io> wrote:
> 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 when using the new
> method due to a mistake. That being said, I think the value added in
> clarity of contract of when the error reporter will be invoked and overall
> aesthetic while maintaining backwards compatibility outweighs the potential
> mistake of a developer in not implementing the original put(...) method.
>
> With respect to synchrony, I agree with Konstantine's point, that we
> should make it an opt-in feature of making the reporter only synchronous.
> At the same time, 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 functionality. I also believe making it opt-in
> synchronous vs. the other way simplifies implementation for the developer
> (blocking vs creating a new thread).
>
> Please let me know your thoughts. I would like to come to a consensus soon
> due to the AK 2.6 deadlines; I will then shortly update the KIP and start a
> vote.
>
> Thanks,
> Aakash
>
> On Fri, May 15, 2020 at 2:24 PM Randall Hauch <rha...@gmail.com> wrote:
>
>> On Fri, May 15, 2020 at 3:13 PM Arjun Satish <arjun.sat...@gmail.com>
>> 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
>> method,
>> > and we are passing the compatibility problems back to the connector
>> > developers.
>> >
>>
>> New connectors would have to implement both if they want to run in older
>> runtimes.
>>
>>
>> > 2. if we deprecate the older put() method and eventually remove it, then
>> > old connectors are forward incompatible. If we are not going to remove
>> it,
>> > then maybe we should not deprecate it?
>> >
>>
>> I don't think we'll ever remove deprecated methods -- there's no reason to
>> cut off older connectors.
>>
>>
>> > 3. if a record is realized to be erroneous outside put() (say, in flush
>> or
>> > preCommit), how will it be reported?
>> >
>>
>> This is a concern no matter how the reporter is passed to the task.
>> Actually, I think it's more clear that the reporter passed through
>> `put(...)` should be used to record errors on the SinkRecords passed in
>> the
>> same method call.
>>
>>
>> >
>> > I do think the concern over aesthetics is an important one, but the
>> > trade-off here is to exclude many connectors that are out there from
>> > running on worker versions. there may be production deployments that
>> need
>> > one old and one new connector that now cannot work on any version of a
>> > single worker. Building connectors is complex, and it's kinda unfair to
>> > expect folks to make changes over aesthetic reasons alone. This is
>> probably
>> > the reason why popular framework APIs very rarely (and probably never)
>> > change.
>> >
>>
>> I don't see how passing the reporter through an overloaded `put(...)` is
>> less backward compatible. Because the runtime provides the SinkTask base
>> class, the runtime has control over what the methods do by default.
>>
>>
>> >
>> > Overall, yes, the "public void
>> errantRecordReporter(BiConsumer<SinkRecord,
>> > Throwable> reporter) {}" proposal in the original KIP is somewhat of a
>> > mouthful, but are there are any simpler alternatives that do not exclude
>> > existing connectors, adding operational burdens and yet provide a clean
>> > contract?
>> >
>>
>> IMO, overloading `put(...)` is cleaner and easier to understand -- plus
>> the
>> other benefits in my earlier email.
>>
>>
>> >
>> > Best,
>> >
>> > PS: Apologies if the language is incorrect or some points are unclear.
>> >
>> > On Fri, May 15, 2020 at 12:02 PM Randall Hauch <rha...@gmail.com>
>> wrote:
>> >
>> > > 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 APIs.
>> > > >
>> > >
>> > > +1
>> > >
>> > >
>> > > > What you describe is easily an opt-in feature for the connector
>> > > developer.
>> > > > At the same time, the latest description above, gives us better
>> chances
>> > > for
>> > > > this API to remain like this for longer, because it covers both the
>> > sync
>> > > > and async per `put` user cases.
>> > >
>> > >
>> > > +1
>> > >
>> > >
>> > > > Given how simple the sync implementation
>> > > > is, just by complying with the return type of the method, I still
>> think
>> > > the
>> > > > BiFunction definition that returns a Future makes sense.
>> > > >
>> > > > Konstantine
>> > > >
>> > > >
>> > > >
>> > > > On Fri, May 15, 2020 at 11:27 AM Aakash Shah <as...@confluent.io>
>> > wrote:
>> > > >
>> > > > > 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 synchronous approach for the
>> error
>> > > > > reporter was to ease the connector developer's job interacting
>> with
>> > and
>> > > > > handling the error reporter. The tradeoff for having a
>> > synchronous-only
>> > > > > reporter would be lower throughput on the reporter; this was
>> thought
>> > to
>> > > > be
>> > > > > fine since arguably most circumstances would not include
>> consistently
>> > > > large
>> > > > > amounts of records being sent to the error reporter. Even if this
>> was
>> > > the
>> > > > > case, an argument can be made that the lower throughput would be
>> of
>> > > > > assistance in this case, as it would allow more time for the user
>> to
>> > > > > realize the connector is having records sent to the error reporter
>> > > before
>> > > > > many are sent. However, if we are strongly in favor of having the
>> > > option
>> > > > of
>> > > > > asynchronous functionality available for the developer, then I am
>> > fine
>> > > > with
>> > > > > that as well.
>> > > > >
>> > > > > Lastly, I am on board with changing the name to
>> failedRecordReporter,
>> > > > >
>> > > > > Please let me know your thoughts.
>> > > > >
>> > > > > Thanks,
>> > > > > Aakash
>> > > > >
>> > > > > On Fri, May 15, 2020 at 9:10 AM Randall Hauch <rha...@gmail.com>
>> > > wrote:
>> > > > >
>> > > > > > 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 this new name may be a bit more precise
>> > and
>> > > > I'd
>> > > > > be
>> > > > > > fine with this.
>> > > > > >
>> > > > > > Second, should the reporter method be synchronous? I think the
>> two
>> > > > > options
>> > > > > > are:
>> > > > > >
>> > > > > > 2a. Use `BiConsumer<SinkRecord, Throwable>` that returns nothing
>> > and
>> > > > > blocks
>> > > > > > (at this time).
>> > > > > > 2b. Use `BiFunction<SinkRecord, Throwable, Future<Void>>` that
>> > > returns
>> > > > a
>> > > > > > future that the user can optionally use to be synchronous.
>> > > > > >
>> > > > > > I do agree with Konstantine that option 2b gives us more room
>> for
>> > > > future
>> > > > > > semantic changes, and since the producer write is already
>> > > asynchronous
>> > > > > this
>> > > > > > should be straightforward to implement. I think the concern
>> here is
>> > > > that
>> > > > > if
>> > > > > > the sink task does not *use* the future to make this
>> synchronous,
>> > it
>> > > is
>> > > > > > very possible that the error records could be written out of
>> order
>> > > (due
>> > > > > to
>> > > > > > retries). But this won't be an issue if the implementation uses
>> > > > > > `max.in.flight.requests.per.connection=1` for writing the error
>> > > > records.
>> > > > > > It's a little less clear, but honestly IMO passing the reporter
>> in
>> > > the
>> > > > > > `put(...)` method helps make this lambda easier to understand,
>> for
>> > > some
>> > > > > > strange reason. So unless there are good reasons to avoid this,
>> I'd
>> > > be
>> > > > in
>> > > > > > favor of 2b and returning a Future.
>> > > > > >
>> > > > > > Third, how do we pass the reporter lambda / method reference to
>> the
>> > > > task?
>> > > > > > My proposal to pass the reporter via an overload `put(...)`
>> still
>> > is
>> > > > the
>> > > > > > most attractive to me, for several reasons:
>> > > > > >
>> > > > > > 3a. There's no need to pass the reporter separately *and* to
>> > describe
>> > > > the
>> > > > > > changes in method call ordering.
>> > > > > > 3b. As mentioned above, for some reason passing it via
>> `put(...)`
>> > > makes
>> > > > > the
>> > > > > > intent more clear that it be used when processing the
>> SinkRecord,
>> > and
>> > > > > that
>> > > > > > it shouldn't be used in `start(...)`, `preCommit(...)`,
>> > > > > > `onPartitionsAssigned(...)`, or any of the other task methods.
>> As
>> > > > Andrew
>> > > > > > pointed out earlier, *describing* this in the KIP and in JavaDoc
>> > will
>> > > > be
>> > > > > > tough to be exact yet succinct.
>> > > > > > 3c. There is already precedence for evolving
>> > > > > > `SourceTask.commitRecord(...)`, and the pattern is identical.
>> > > > > > 3d. Backward compatibility is easy to understand, and at the
>> same
>> > > time
>> > > > > it's
>> > > > > > pretty easy to describe what implementations that want to take
>> > > > advantage
>> > > > > of
>> > > > > > this feature should do.
>> > > > > > 3e. Minimal changes to the interface: we're just *adding* one
>> > default
>> > > > > > method that calls the existing method and deprecating the
>> existing
>> > > > > > `put(...)`.
>> > > > > > 3f. Deprecating the existing `put(...)` makes it more clear in a
>> > > > > > programmatic sense that new sink implementations should use the
>> > > > reporter,
>> > > > > > and that we recommend old sinks evolve to use it.
>> > > > > >
>> > > > > > Some of these benefits apply to some of the other suggestions,
>> but
>> > I
>> > > > > think
>> > > > > > none of the other suggestions have all of these benefits. For
>> > > example,
>> > > > > > overloading `initialize(...)` is more difficult since most sink
>> > > > > connectors
>> > > > > > don't override it and therefore would be less subject to
>> > deprecations
>> > > > > > warnings. Overloading `start(...)` is less attractive. Adding a
>> > > method
>> > > > > IMO
>> > > > > > shares the fewest of these benefits.
>> > > > > >
>> > > > > > The one disadvantage of this approach is that sink task
>> > > implementations
>> > > > > > can't rely upon the reporter upon startup. IMO that's an
>> acceptable
>> > > > > > tradeoff to get the cleaner and more explicit API, especially if
>> > the
>> > > > API
>> > > > > > contract is that Connect will pass the same reporter instance to
>> > each
>> > > > > call
>> > > > > > to `put(...)` on a single task instance.
>> > > > > >
>> > > > > > Best regards,
>> > > > > >
>> > > > > > Randall
>> > > > > >
>> > > > > > On Fri, May 15, 2020 at 6:59 AM Andrew Schofield <
>> > > > > > andrew_schofi...@live.com>
>> > > > > > wrote:
>> > > > > >
>> > > > > > > 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" <as...@confluent.io>
>> wrote:
>> > > > > > >
>> > > > > > > > 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 break if the old
>> > > > put(...)
>> > > > > > > > doesn't correctly invoke the overloaded put(...); either
>> that,
>> > or
>> > > > it
>> > > > > > will
>> > > > > > > > add duplication of functionality across the two put(...)
>> > > methods. I
>> > > > > > think
>> > > > > > > > the older method simplifies things with the idea that a
>> > DLQ/error
>> > > > > > > reporter
>> > > > > > > > will or will not be passed into the method depending on the
>> > > version
>> > > > > of
>> > > > > > > AK.
>> > > > > > > > However, I also understand the aesthetic advantage of this
>> > method
>> > > > vs
>> > > > > > the
>> > > > > > > > setter method, so I am okay with going in this direction if
>> > > others
>> > > > > > agree
>> > > > > > > > with adding the overloaded put(...).
>> > > > > > > >
>> > > > > > > > 2. Yes, your assumption is correct. Yes, we can remove the
>> > "Order
>> > > > of
>> > > > > > > > Operations" if we go with the overloaded put(...) direction.
>> > > > > > > >
>> > > > > > > > 3. Great point, I will remove them from the KIP.
>> > > > > > > >
>> > > > > > > > 4. Yeah, accept(...) will be synchronous. I will change it
>> to
>> > be
>> > > > > > clearer,
>> > > > > > > > thanks.
>> > > > > > > >
>> > > > > > > > 5. This KIP will use existing metrics as well introduce new
>> > > > metrics.
>> > > > > I
>> > > > > > > will
>> > > > > > > > update this section to fully specify the metrics.
>> > > > > > > >
>> > > > > > > > Please let me know what you think.
>> > > > > > > >
>> > > > > > > > Thanks,
>> > > > > > > > Aakash
>> > > > > > > >
>> > > > > > > > On Thu, May 14, 2020 at 3:52 PM Randall Hauch <
>> > rha...@gmail.com>
>> > > > > > wrote:
>> > > > > > > >
>> > > > > > > > > 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 this feature when deployed in a
>> runtime
>> > > > that
>> > > > > > > supports
>> > > > > > > > > this feature, but can safely and easily do without the
>> > feature
>> > > > when
>> > > > > > > > > deployed to an older runtime. But I do understand Andrew's
>> > > > concern
>> > > > > > > about
>> > > > > > > > > the aesthetics. Have you considered overloading the
>> > `put(...)`
>> > > > > method
>> > > > > > > and
>> > > > > > > > > adding the `reporter` as a second parameter? Essentially
>> it
>> > > would
>> > > > > add
>> > > > > > > the
>> > > > > > > > > one method (with proper JavaDoc) to `SinkTask` only:
>> > > > > > > > >
>> > > > > > > > > ```
>> > > > > > > > > public void put(Collection<SinkRecord> records,
>> > > > > > > BiFunction<SinkRecord,
>> > > > > > > > > Throwable> reporter) {
>> > > > > > > > > put(records);
>> > > > > > > > > }
>> > > > > > > > > ```
>> > > > > > > > > and the WorkerSinkTask would be changed to call
>> > > `put(Collection,
>> > > > > > > > > BiFunction)` instead.
>> > > > > > > > >
>> > > > > > > > > Sink connector implementations that don't do anything
>> > different
>> > > > can
>> > > > > > > still
>> > > > > > > > > override `put(Collection)`, and it still works as before.
>> > > > > Developers
>> > > > > > > that
>> > > > > > > > > want to change their sink connector implementations to
>> > support
>> > > > this
>> > > > > > new
>> > > > > > > > > feature would do the following, which would work in older
>> and
>> > > > newer
>> > > > > > > Connect
>> > > > > > > > > runtimes:
>> > > > > > > > > ```
>> > > > > > > > > public void put(Collection<SinkRecord> records) {
>> > > > > > > > > put(records, null);
>> > > > > > > > > }
>> > > > > > > > > public void put(Collection<SinkRecord> records,
>> > > > > > > BiFunction<SinkRecord,
>> > > > > > > > > Throwable> reporter) {
>> > > > > > > > > // the normal `put(Collection)` logic goes here,
>> but
>> > > can
>> > > > > > > optionally
>> > > > > > > > > use `reporter` if non-null
>> > > > > > > > > }
>> > > > > > > > > ```
>> > > > > > > > >
>> > > > > > > > > I think this has all the same benefits of the current KIP,
>> > but
>> > > > > > > > > it's noticeably simpler and hopefully more aesthetically
>> > > > pleasing.
>> > > > > > > > >
>> > > > > > > > > As for Andrew's second concern about "the task can send
>> > errant
>> > > > > > records
>> > > > > > > to
>> > > > > > > > > it within put(...)" being too restrictive. My guess is
>> that
>> > > this
>> > > > > was
>> > > > > > > more
>> > > > > > > > > an attempt at describing the basic behavior, and less
>> about
>> > > > > requiring
>> > > > > > > the
>> > > > > > > > > reporter only being called within the `put(...)` method
>> and
>> > not
>> > > > by
>> > > > > > > methods
>> > > > > > > > > to which `put(...)` synchronously or asynchronously
>> > delegates.
>> > > > Can
>> > > > > > you
>> > > > > > > > > confirm whether my assumption is correct? If so, then
>> perhaps
>> > > my
>> > > > > > > suggestion
>> > > > > > > > > helps work around this issue as well, since there would
>> be no
>> > > > > > > restriction
>> > > > > > > > > on when the reporter is called, and the whole "Order of
>> > > > Operations"
>> > > > > > > section
>> > > > > > > > > could potentially be removed.
>> > > > > > > > >
>> > > > > > > > > Third, it's not clear to me why the "Error Reporter
>> Object"
>> > > > > > subsection
>> > > > > > > in
>> > > > > > > > > the "Proposal" section lists the worker configuration
>> > > properties
>> > > > > that
>> > > > > > > were
>> > > > > > > > > previously introduced with
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-298%3A+Error+Handling+in+Connect
>> > > > > > > > > .
>> > > > > > > > > Maybe it's worth mentioning that the error reporter
>> > > functionality
>> > > > > > will
>> > > > > > > > > reuse or build upon KIP-298, including reusing the
>> > > configuration
>> > > > > > > properties
>> > > > > > > > > defined in KIP-298. But IIUC, the KIP does not propose
>> > changing
>> > > > any
>> > > > > > > > > technical or semantic aspect of these configuration
>> > properties,
>> > > > and
>> > > > > > > > > therefore the KIP would be more clear and succinct without
>> > > them.
>> > > > > > > *That* the
>> > > > > > > > > error reporter will use these properties is part of the UX
>> > and
>> > > > > > > therefore
>> > > > > > > > > necessary to mention, but *how* it uses those properties
>> is
>> > > > really
>> > > > > up
>> > > > > > > to
>> > > > > > > > > the implementation.
>> > > > > > > > >
>> > > > > > > > > Fourth, the "Synchrony" section has a sentence that is
>> > > confusing,
>> > > > > or
>> > > > > > > not as
>> > > > > > > > > clear as it could be.
>> > > > > > > > >
>> > > > > > > > > "If a record is sent to the error reporter,
>> processing of
>> > > the
>> > > > > > next
>> > > > > > > > > errant record in accept(...) will not begin until the
>> > producer
>> > > > > > > successfully
>> > > > > > > > > sends the errant record to Kafka."
>> > > > > > > > >
>> > > > > > > > > This sentence is a bit difficult to understand, but IIUC
>> this
>> > > > > really
>> > > > > > > just
>> > > > > > > > > means that "accept(...)" will be synchronous and will
>> block
>> > > until
>> > > > > the
>> > > > > > > > > errant record has been successfully written to Kafka. If
>> so,
>> > > > let's
>> > > > > > say
>> > > > > > > > > that. The rest of the paragraph is fine.
>> > > > > > > > >
>> > > > > > > > > Finally, is this KIP proposing new metrics, or that
>> existing
>> > > > > metrics
>> > > > > > > would
>> > > > > > > > > be used to track the error reporter usage? If the former,
>> > then
>> > > > > please
>> > > > > > > > > fully-specify what these metrics will be, similarly to how
>> > > > metrics
>> > > > > > are
>> > > > > > > > > specified in
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-196%3A+Add+metrics+to+Kafka+Connect+framework
>> > > > > > > > > .
>> > > > > > > > >
>> > > > > > > > > Thoughts?
>> > > > > > > > >
>> > > > > > > > > Best regards,
>> > > > > > > > >
>> > > > > > > > > Randall
>> > > > > > > > >
>> > > > > > > > > On Mon, May 11, 2020 at 4:49 PM Andrew Schofield <
>> > > > > > > > > andrew_schofi...@live.com>
>> > > > > > > > > wrote:
>> > > > > > > > >
>> > > > > > > > > > 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() method is that it should send the records
>> > > > > > > asynchronously
>> > > > > > > > > > and immediately
>> > > > > > > > > > return, so the task is likely to want to report errors
>> > > > > > asynchronously
>> > > > > > > > > > too. Currently the KIP
>> > > > > > > > > > states that "the task can send errant records to it
>> within
>> > > > > > put(...)"
>> > > > > > > and
>> > > > > > > > > > that's too restrictive.
>> > > > > > > > > > The task ought to be able to report any unflushed
>> records,
>> > > but
>> > > > > the
>> > > > > > > > > > synchronisation of this is going
>> > > > > > > > > > to be tricky. I suppose the connector author needs to
>> make
>> > > sure
>> > > > > > that
>> > > > > > > all
>> > > > > > > > > > errant records have
>> > > > > > > > > > been reported before returning control from
>> > > SinkTask.flush(...)
>> > > > > or
>> > > > > > > > > perhaps
>> > > > > > > > > > SinkTask.preCommit(...).
>> > > > > > > > > >
>> > > > > > > > > > I think the interface is a little strange too. I can see
>> > that
>> > > > > this
>> > > > > > > was
>> > > > > > > > > > done so it's possible to deliver a connector
>> > > > > > > > > > that supports error reporting but it can also work in
>> > earlier
>> > > > > > > versions of
>> > > > > > > > > > the KC runtime. But, the
>> > > > > > > > > > pattern so far is that the task uses the methods of
>> > > > > SinkTaskContext
>> > > > > > > to
>> > > > > > > > > > access utilities in the Kafka
>> > > > > > > > > > Connect runtime, and I suggest that reporting a bad
>> record
>> > is
>> > > > > such
>> > > > > > a
>> > > > > > > > > > utility. SinkTaskContext has
>> > > > > > > > > > changed before when the configs() methods was added, so
>> I
>> > > think
>> > > > > > > there is
>> > > > > > > > > > precedent for adding a method.
>> > > > > > > > > > The way the KIP adds a method to SinkTask that the KC
>> > runtime
>> > > > > calls
>> > > > > > > to
>> > > > > > > > > > provide the error reporting utility
>> > > > > > > > > > seems not to match what has gone before.
>> > > > > > > > > >
>> > > > > > > > > > Thanks,
>> > > > > > > > > > Andrew
>> > > > > > > > > >
>> > > > > > > > > > On 11/05/2020, 19:05, "Aakash Shah" <as...@confluent.io
>> >
>> > > > wrote:
>> > > > > > > > > >
>> > > > > > > > > > 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
>> > > > > > the
>> > > > > > > > > > backwards
>> > > > > > > > > > compatibility of the initial approach, please let me
>> > know
>> > > > > what
>> > > > > > > you
>> > > > > > > > > > think.
>> > > > > > > > > >
>> > > > > > > > > > Thanks,
>> > > > > > > > > > Aakash
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> ____________________________________________________________________________________________________________________________
>> > > > > > > > > >
>> > > > > > > > > > Hi,
>> > > > > > > > > > By adding a new method to the SinkContext interface
>> in
>> > > say
>> > > > > > Kafka
>> > > > > > > > > 2.6, a
>> > > > > > > > > > connector that calls it would require a Kafka 2.6
>> > connect
>> > > > > > > runtime. I
>> > > > > > > > > > don't
>> > > > > > > > > > quite see how that's a backward compatibility
>> problem.
>> > > It's
>> > > > > > just
>> > > > > > > that
>> > > > > > > > > > new
>> > > > > > > > > > connectors need the latest interface. I might not
>> quite
>> > > be
>> > > > > > > > > > understanding,
>> > > > > > > > > > but I think it would be fine.
>> > > > > > > > > >
>> > > > > > > > > > Thanks,
>> > > > > > > > > > Andrew
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> ____________________________________________________________________________________________________________________________
>> > > > > > > > > >
>> > > > > > > > > > Hi Andrew,
>> > > > > > > > > >
>> > > > > > > > > > I apologize for the way the reply was sent. I just
>> > > > subscribed
>> > > > > > to
>> > > > > > > the
>> > > > > > > > > > dev
>> > > > > > > > > > mailing list so it should be resolved now.
>> > > > > > > > > >
>> > > > > > > > > > You are correct, new connectors would simply require
>> > the
>> > > > > latest
>> > > > > > > > > > interface.
>> > > > > > > > > > However, we want to remove that requirement - in
>> other
>> > > > words,
>> > > > > > we
>> > > > > > > want
>> > > > > > > > > > to
>> > > > > > > > > > allow the possibility that someone wants the latest
>> > > > > > connector/to
>> > > > > > > > > > upgrade to
>> > > > > > > > > > the latest version, but deploys it on an older
>> version
>> > of
>> > > > AK.
>> > > > > > > > > > Basically, we
>> > > > > > > > > > don't want to enforce the necessity of upgrading AK
>> to
>> > > get
>> > > > > the
>> > > > > > > latest
>> > > > > > > > > > interface. In the current approach, there would be
>> no
>> > > issue
>> > > > > of
>> > > > > > > > > > deploying a
>> > > > > > > > > > new connector on an older version of AK, as the
>> Connect
>> > > > > > framework
>> > > > > > > > > would
>> > > > > > > > > > simply not invoke the new method.
>> > > > > > > > > >
>> > > > > > > > > > Please let me know what you think and if I need to
>> > > clarify
>> > > > > > > anything.
>> > > > > > > > > >
>> > > > > > > > > > Thanks,
>> > > > > > > > > > Aakash
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
