Hi Chia-Ping, Deprecating `send` is going to be extremely disruptive to all existing users (if you use -Werror, it will require updating every call site). Have we considered encouraging the usage of the new method while not deprecating the old methods? We could consider deprecation down the line. The existing methods work fine for many people, it doesn't seem like a good idea to penalize them.
Instead, we can make the new method available for people who benefit from it. After a grace period (3 releases), we can consider deprecating. Thoughts? Ismael On Tue, Mar 30, 2021 at 8:50 PM Chia-Ping Tsai <chia7...@apache.org> wrote: > hi, > > I have updated KIP according to my latest response. I will start vote > thread next week if there is no more comments :) > > Best Regards, > Chia-Ping > > On 2021/01/31 05:39:17, Chia-Ping Tsai <chia7...@apache.org> wrote: > > It seems to me changing the input type might make complicate the > migration from deprecated send method to new API. > > > > Personally, I prefer to introduce a interface called “SendRecord” to > replace ProducerRecord. Hence, the new API/classes is shown below. > > > > 1. CompletionStage send(SendRecord) > > 2. class ProducerRecord implement SendRecord > > 3. Introduce builder pattern for SendRecord > > > > That includes following benefit. > > > > 1. Kafka users who don’t use both return type and callback do not need > to change code even though we remove deprecated send methods. (of course, > they still need to compile code with new Kafka) > > > > 2. Kafka users who need Future can easily migrate to new API by regex > replacement. (cast ProduceRecord to SendCast and add toCompletableFuture) > > > > 3. It is easy to support topic id in the future. We can add new method > to SendRecord builder. For example: > > > > Builder topicName(String) > > Builder topicId(UUID) > > > > 4. builder pattern can make code more readable. Especially, Produce > record has a lot of fields which can be defined by users. > > — > > Chia-Ping > > > > On 2021/01/30 22:50:36 Ismael Juma wrote: > > > Another thing to think about: the consumer api currently has > > > `subscribe(String|Pattern)` and a number of methods that accept > > > `TopicPartition`. A similar approach could be used for the Consumer to > work > > > with topic ids or topic names. The consumer side also has to support > > > regexes so it probably makes sense to have a separate interface. > > > > > > Ismael > > > > > > On Sat, Jan 30, 2021 at 2:40 PM Ismael Juma <ism...@juma.me.uk> wrote: > > > > > > > I think this is a promising idea. I'd personally avoid the overload > and > > > > simply have a `Topic` type that implements `SendTarget`. It's a mix > of both > > > > proposals: strongly typed, no overloads and general class names that > > > > implement `SendTarget`. > > > > > > > > Ismael > > > > > > > > On Sat, Jan 30, 2021 at 2:22 PM Jason Gustafson <ja...@confluent.io> > > > > wrote: > > > > > > > >> Giving this a little more thought, I imagine sending to a topic is > the > > > >> most > > > >> common case, so maybe it's an overload worth having. Also, if > `SendTarget` > > > >> is just a marker interface, we could let `TopicPartition` implement > it > > > >> directly. Then we have: > > > >> > > > >> interface SendTarget; > > > >> class TopicPartition implements SendTarget; > > > >> > > > >> CompletionStage<RecordMetadata> send(String topic, Record record); > > > >> CompletionStage<RecordMetadata> send(SendTarget target, Record > record); > > > >> > > > >> The `SendTarget` would give us a lot of flexibility in the future. > It > > > >> would > > > >> give us a couple options for topic ids. We could either have an > overload > > > >> of > > > >> `send` which accepts `Uuid`, or we could add a `TopicId` type which > > > >> implements `SendTarget`. > > > >> > > > >> -Jason > > > >> > > > >> > > > >> On Sat, Jan 30, 2021 at 1:11 PM Jason Gustafson <ja...@confluent.io > > > > > >> wrote: > > > >> > > > >> > Yeah, good question. I guess we always tend to regret using > lower-level > > > >> > types in these APIs. Perhaps there should be some kind of > interface: > > > >> > > > > >> > interface SendTarget > > > >> > class TopicIdTarget implements SendTarget > > > >> > class TopicTarget implements SendTarget > > > >> > class TopicPartitionTarget implements SendTarget > > > >> > > > > >> > Then we just have: > > > >> > > > > >> > CompletionStage<RecordMetadata> send(SendTarget target, Record > record); > > > >> > > > > >> > Not sure if we could reuse `Record` in the consumer though. We do > have > > > >> > some state in `ConsumerRecord` which is not present in > `ProducerRecord` > > > >> > (e.g. offset). Perhaps we could provide a `Record` view from > > > >> > `ConsumerRecord` for convenience. That would be useful for use > cases > > > >> which > > > >> > involve reading from one topic and writing to another. > > > >> > > > > >> > -Jason > > > >> > > > > >> > On Sat, Jan 30, 2021 at 12:29 PM Ismael Juma <ism...@juma.me.uk> > wrote: > > > >> > > > > >> >> Interesting idea. A couple of things to consider: > > > >> >> > > > >> >> 1. Would we introduce the Message concept to the Consumer too? I > think > > > >> >> that's what .NET does. > > > >> >> 2. If we eventually allow a send to a topic id instead of topic > name, > > > >> >> would > > > >> >> that result in two additional overloads? > > > >> >> > > > >> >> Ismael > > > >> >> > > > >> >> On Sat, Jan 30, 2021 at 11:38 AM Jason Gustafson < > ja...@confluent.io> > > > >> >> wrote: > > > >> >> > > > >> >> > For the sake of having another option to shoot down, we could > take a > > > >> >> page > > > >> >> > from the .net client and separate the message data from the > > > >> destination > > > >> >> > (i.e. topic or partition). This would get around the need to > use a > > > >> new > > > >> >> > verb. For example: > > > >> >> > > > > >> >> > CompletionStage<RecordMetadata> send(String topic, Message > message); > > > >> >> > CompletionStage<RecordMetadata> send(TopicPartition > topicPartition, > > > >> >> Message > > > >> >> > message); > > > >> >> > > > > >> >> > -Jason > > > >> >> > > > > >> >> > > > > >> >> > > > > >> >> > On Sat, Jan 30, 2021 at 11:30 AM Jason Gustafson < > ja...@confluent.io > > > >> > > > > >> >> > wrote: > > > >> >> > > > > >> >> > > I think this still makes sense as a separate KIP. For > KIP-691, we > > > >> are > > > >> >> > just > > > >> >> > > looking to help define the error contract for the new API. > > > >> >> > > > > > >> >> > > -Jason > > > >> >> > > > > > >> >> > > On Sat, Jan 30, 2021 at 8:39 AM Ismael Juma < > ism...@juma.me.uk> > > > >> >> wrote: > > > >> >> > > > > > >> >> > >> Are we saying that we won't pursue this KIP in favor of the > other > > > >> >> one? > > > >> >> > >> > > > >> >> > >> Ismael > > > >> >> > >> > > > >> >> > >> On Sat, Jan 30, 2021, 4:15 AM Chia-Ping Tsai < > chia7...@apache.org > > > >> > > > > >> >> > wrote: > > > >> >> > >> > > > >> >> > >> > hi Jason > > > >> >> > >> > > > > >> >> > >> > Thanks for your response. "transmit" is good to me. > > > >> >> > >> > > > > >> >> > >> > As we discussed by email, KIP-706 is going to be merged to > > > >> KIP-691( > > > >> >> > >> > https://cwiki.apache.org/confluence/x/PSfZCQ). Hence, > please > > > >> feel > > > >> >> > free > > > >> >> > >> to > > > >> >> > >> > replace "produce" by "transmit" in KIP-691. > > > >> >> > >> > > > > >> >> > >> > Best, > > > >> >> > >> > Chia-Ping > > > >> >> > >> > > > > >> >> > >> > On 2021/01/30 00:48:38, Jason Gustafson < > ja...@confluent.io> > > > >> >> wrote: > > > >> >> > >> > > Hi Chia-Ping, > > > >> >> > >> > > > > > >> >> > >> > > I think this is a great idea. It is a pity that we > cannot > > > >> >> continue > > > >> >> > to > > > >> >> > >> use > > > >> >> > >> > > the `send` verb, but I don't see how we can. I know we > > > >> considered > > > >> >> > >> > > `transmit` as another option which is closer to `send`. > That > > > >> >> would > > > >> >> > >> avoid > > > >> >> > >> > > the redundancy when people choose the common "producer" > > > >> variable > > > >> >> > name. > > > >> >> > >> > > > > > >> >> > >> > > producer.transmit > > > >> >> > >> > > > > > >> >> > >> > > instead of > > > >> >> > >> > > > > > >> >> > >> > > producer.produce > > > >> >> > >> > > > > > >> >> > >> > > A couple alternatives might be `write` or `append`. I'm > happy > > > >> >> with > > > >> >> > >> > > `produce` as well, but curious if others have thoughts. > > > >> >> > >> > > > > > >> >> > >> > > -Jason > > > >> >> > >> > > > > > >> >> > >> > > > > > >> >> > >> > > On Wed, Jan 20, 2021 at 9:37 AM Chia-Ping Tsai < > > > >> >> chia7...@apache.org > > > >> >> > > > > > >> >> > >> > wrote: > > > >> >> > >> > > > > > >> >> > >> > > > Dear all, > > > >> >> > >> > > > > > > >> >> > >> > > > I'd like to start the discussion thread for KIP-706: > > > >> >> > >> > > > > > > >> >> > >> > > > > >> >> > >> > > > >> >> > > > > >> >> > > > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100829459 > > > >> >> > >> > > > > > > >> >> > >> > > > KIP-706 is proposing to introduce new API > "CompletionStage > > > >> >> > >> > > > produce(record)" to Producer. Kafka users can leverage > > > >> >> > >> CompletionStage > > > >> >> > >> > to > > > >> >> > >> > > > write asynchronous non-blocking code. CompletionStage > is > > > >> more > > > >> >> > >> powerful > > > >> >> > >> > than > > > >> >> > >> > > > Future and callback. Also, the code using Future and > > > >> callback > > > >> >> can > > > >> >> > be > > > >> >> > >> > easily > > > >> >> > >> > > > re-written by CompletionStage. > > > >> >> > >> > > > > > > >> >> > >> > > > Cheers, > > > >> >> > >> > > > Chia-Ping > > > >> >> > >> > > > > > > >> >> > >> > > > > > > >> >> > >> > > > > > >> >> > >> > > > > >> >> > >> > > > >> >> > > > > > >> >> > > > > >> >> > > > >> > > > > >> > > > > > > > > > >
