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 the futures at once in a batch at the end, why not support batch processing in a future KIP, since it is not too difficult now that we are adding an interface. I'm not sure I see any gain beyond some user control that could increase throughput - but at the same time, as I mentioned before, I don't think throughput is a factor we need to consider much with error reporting. We don't really need or necessarily want a higher throughput on error reporting, as ideally, there should not be a high volume of errant records. Thanks, Aakash On Mon, May 18, 2020 at 1:22 PM Chris Egerton <chr...@confluent.io> wrote: > 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 anyway, > why make it a future at all? We should have some idea of how people would > use a future that doesn't just hinge on them blocking on it immediately, > and isn't more easily-addressed by a different API (such as one with batch > reporting). > > Cheers, > > Chris > > On Mon, May 18, 2020 at 1:17 PM Aakash Shah <as...@confluent.io> wrote: > > > 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 are introducing a new interface), we should > > provide functionality that we know will provide value immediately and not > > cause any developer/user burden. > > > > In that case, I think the main area we have to come to a consensus on is > - > > how much control do we want to provide to the developer/user in this KIP > > considering that we can add the functionality relatively easily later? > > > > Randall, Konstantine, what do you think about adding it later vs now? > > > > Thanks, > > Aakash > > > > On Mon, May 18, 2020 at 12:45 PM Chris Egerton <chr...@confluent.io> > > wrote: > > > > > 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 of handling records is still blocking, and to be done > > > truly asynchronously without blocking processing of non-errant records, > > > would have to be done on a separate thread. It's technically possible > for > > > users to cache all of these futures and instead of invoking "get" on > > them, > > > simply check whether they're complete or not via "isDone", but this > seems > > > like an anti-pattern. > > > > > > > What is the benefit of wrapping this in a future? > > > > > > As far as I can tell, there hasn't been a practical example yet where > the > > > flexibility provided by a future would actually be beneficial in > writing > > a > > > connector. It'd be great if we could find one. One possible use case > > might > > > be processing records received in "SinkTask::put" without having to > block > > > for each errant record report before sending non-errant records to the > > > sink. However, this could also be addressed by allowing for batch > > reporting > > > of errant records instead of accepting a single record at a time; the > > task > > > would track errant records as it processes them in "put" and report > them > > > all en-masse after all non-errant records have been processed. > > > > > > With regards to the precedent of using futures for asynchronous APIs, I > > > think we should make sure that whatever API we decide on is actually > > useful > > > for the cases it serves. There's plenty of precedent for callback-based > > > asynchronous APIs in Kafka with both "Producer::send" and > > > "Consumer::commitAsync"; the question here shouldn't be about what's > done > > > in different APIs, but what would work for this one in particular. > > > > > > Finally, it's also been brought up that if we're going to introduce a > new > > > error reporter interface, we can always modify that interface later on > to > > > go from asynchronous to synchronous behavior, or vice-versa, or even to > > add > > > a callback- or future-based variant that didn't exist before. We have > > > plenty of room to maneuver in the future here, so the pressure to get > > > everything right the first time and provide maximum flexibility doesn't > > > seem as pressing, and the goal of minimizing the kind of API that we > have > > > to support for future versions without making unnecessary additions is > > > easier to achieve. > > > > > > Cheers, > > > > > > Chris > > > > > > > > > > > > On Mon, May 18, 2020 at 12:20 PM Aakash Shah <as...@confluent.io> > wrote: > > > > > > > Hi Arjun, > > > > > > > > Thanks for your feedback. > > > > > > > > I agree with moving to Future<Void>, 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 Connect functionality is already > > > > asynchronous, I am inclined to believe that customers will want an > > > > asynchronous solution for this as well. And if is relatively simple > to > > > > block with future.get() to make it synchronous, would you not say > that > > > > having an opt-in synchronous functionality rather than synchronous > only > > > > functionality allows for customer control while maintaining that not > > too > > > > much burden of implementation is placed on the customer? > > > > WDYT? > > > > > > > > Thanks, > > > > Aakash > > > > > > > > On Sun, May 17, 2020 at 2:51 PM Arjun Satish <arjun.sat...@gmail.com > > > > > > wrote: > > > > > > > > > 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 couple of things: > > > > > > > > > > 1. at a fundamental level, it is a signal to the framework that a > > > failure > > > > > occurred when processing records, specifically due to the given > > record. > > > > > 2. depending on whether errors.log and errors.deadletterqueue has > > been > > > > set, > > > > > some messages are written to zero or more destinations. > > > > > 3. depending on the value of errors.tolerance (none or all), the > task > > > is > > > > > failed after reporters have completed. > > > > > > > > > > for kip-610, the asynchronous method has the advantage of working > > with > > > > the > > > > > internal dead letter queue (which has been transparent to the > > developer > > > > so > > > > > far). but, how does async method help if the DLQ is not enabled? in > > > this > > > > > case RecordMetadata is not very useful, AFAICT? also, if we add > more > > > > error > > > > > reporters in the future (say, for example, a new reporter in a > future > > > > that > > > > > writes to a RDBMS), would the async version return success on all > or > > > > > nothing, and what about partial successes? > > > > > > > > > > overall, if we really need async behavior, I'd prefer to just use > > > > > Future<Void>. but if we can keep it simple, then let's go with a > > > > > synchronous function with the parameters Randall proposed above > (with > > > > > return type as void, and if any of the reporters fail, the task is > > > failed > > > > > if error.tolerance is none, and kept alive if tolerance is all), > and > > > > maybe > > > > > add asynchronous methods in a future KIP? > > > > > > > > > > Best, > > > > > > > > > > > > > > >
