Jun, Yes I'm aware this is allowed but I feel like it's a bit cheating =) Anyway it's a relatively minor KIP so let's do it, +1 binding
David, 1) I understand compatibility is important but I feel like supporting both the old and new interface is excessive. The console tool does not mention the interface and only says a class can be passed so it's not really exposed. 2) I think it's a good idea, I'll update the KIP 3) Yes this is the plan and what I did in the PR. I can see it's not explicitly said in the KIP, I'll fix that Thanks On Wed, Jun 10, 2020 at 8:24 PM David Jacot <dja...@confluent.io> wrote: > > Hi Mickael, > > Thanks for the KIP. That looks very useful. > > I have few small comments/suggestions: > > 1. I was about the make a similar suggestion than Konstantine did regarding > requiring to recompile old formatters. While the formatters are not > directly part of the public API, I think that we can argue that, as they > are accepted by the console consumer, they are somehow part of it. It is a > bit a gray zone. With this in mind, I lean towards supporting both > interfaces by instantiating one or the other instead of making the old one > implements the new one. It is nicer for our users and require a similar > effort to implement overall. > > 2. Should the interface implement Configurable and Closable with default > empty implementations? That would make it similar to our other interfaces > but I am not entirely sure that works with the properties. > > 3. Should we directly move the existing Formatters to using the new > interface? > > Regards, > David > > > > > > Le mer. 10 juin 2020 à 19:13, Maulin Vasavada <maulin.vasav...@gmail.com> a > écrit : > > > +1 (non-binding) > > > > On Wed, Jun 10, 2020 at 9:47 AM Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Bumping this thread. Let me know if you have any questions or feedback. > > > > > > So far, we have 2 binding and 5 non-binding votes. > > > > > > Thanks > > > > > > On Tue, May 19, 2020 at 10:56 AM Manikumar <manikumar.re...@gmail.com> > > > wrote: > > > > > > > > +1 (binding) > > > > > > > > Thanks for the KIP. > > > > > > > > On Tue, May 19, 2020 at 2:57 PM Mickael Maison < > > mickael.mai...@gmail.com > > > > > > > > wrote: > > > > > > > > > Thanks Konstantine for the feedback (and vote)! > > > > > > > > > > 1) I've added example commands using the new formatters > > > > > > > > > > 2) I updated the Compatiblity section to be more explicit about the > > > > > need for recompilation > > > > > > > > > > 3) Good point, updated > > > > > > > > > > On Tue, May 19, 2020 at 3:18 AM Konstantine Karantasis > > > > > <konstant...@confluent.io> wrote: > > > > > > > > > > > > Thanks Michael. > > > > > > I think it's useful to enable specialized message formatters by > > > adding > > > > > this > > > > > > interface to the public API. > > > > > > > > > > > > You have my vote: +1 (binding) > > > > > > > > > > > > Just a few optional comments below: > > > > > > > > > > > > 1. Would you mind adding the equivalent command line example in the > > > > > places > > > > > > where you have an example output? > > > > > > > > > > > > Something equivalent to > > > > > > ./bin/kafka-console-consumer.sh --bootstrap-server localhost:9092 > > > --topic > > > > > > __consumer_offsets --from-beginning --formatter > > > > > > > > > > > > > > > > "kafka.coordinator.group.GroupMetadataManager\$GroupMetadataMessageFormatter" > > > > > > > > > > > > but with the equivalent formatter classes and expected topic names. > > > > > > > > > > > > 2. I have to note that breaking old formatters by requiring > > > recompilation > > > > > > could be avoided if we didn't change kafka.common.MessageFormatter > > to > > > > > > extend the new org.apache.kafka.common.MessageFormatter. We could > > > > > maintain > > > > > > both, while the old one would remain deprecated and we could > > attempt > > > to > > > > > > instantiate one or the other type when reading the config and use > > > either > > > > > of > > > > > > the two different types in the few places in ConsoleConsumer that a > > > > > > formatter is used. But I admit that for this use case, it's not > > worth > > > > > > maintaining both types. The interface wasn't public before anyways. > > > > > > > > > > > > Given that, my small request would be to rephrase in the > > > compatibility > > > > > > section to say something like: > > > > > > 'Existing MessageFormatters implementations will require no changes > > > > > beyond > > > > > > recompilation.' or similar. Because, to be precise, existing > > > formatters > > > > > > _won't_ work if they are given as a parameter to a 2.6 console > > > consumer, > > > > > > without recompilation as you mention. > > > > > > > > > > > > 3. Finally, a minor comment on skipping the use of the `public` > > > specifier > > > > > > in the interface because it's redundant. > > > > > > > > > > > > Best regards, > > > > > > Konstantine > > > > > > > > > > > > On Mon, May 18, 2020 at 3:26 PM Maulin Vasavada < > > > > > maulin.vasav...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > +1 (non-binding) > > > > > > > > > > > > > > On Mon, May 18, 2020 at 8:49 AM Mickael Maison < > > > > > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Bumping this thread as KIP freeze is approaching. > > > > > > > > > > > > > > > > It's a pretty small change and I have a PR ready: > > > > > > > > https://github.com/apache/kafka/pull/8604 > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > On Mon, May 4, 2020 at 5:26 PM Ryanne Dolan < > > > ryannedo...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > +1, non-binding > > > > > > > > > > > > > > > > > > On Mon, May 4, 2020, 9:24 AM Christopher Egerton < > > > > > chr...@confluent.io> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > +1 (non-binding) > > > > > > > > > > > > > > > > > > > > On Mon, May 4, 2020 at 5:02 AM Edoardo Comar < > > > eco...@uk.ibm.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > +1 (non-binding) > > > > > > > > > > > Thanks Mickael > > > > > > > > > > > > > > > > > > > > > > -------------------------------------------------- > > > > > > > > > > > > > > > > > > > > > > Edoardo Comar > > > > > > > > > > > > > > > > > > > > > > Event Streams for IBM Cloud > > > > > > > > > > > IBM UK Ltd, Hursley Park, SO21 2JN > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Mickael Maison <mickael.mai...@gmail.com> > > > > > > > > > > > To: dev <dev@kafka.apache.org> > > > > > > > > > > > Date: 04/05/2020 11:45 > > > > > > > > > > > Subject: [EXTERNAL] [VOTE] KIP-597: MirrorMaker2 > > > > > internal > > > > > > > > topics > > > > > > > > > > > Formatters > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > I'd like to start a vote on KIP-597: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__cwiki.apache.org_confluence_display_KAFKA_KIP-2D597-253A-2BMirrorMaker2-2Binternal-2Btopics-2BFormatters&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=r-_T9EFUWNEUGi1GuX7klXNZIH2sJmxGTtySV3lAjoQ&s=iyBSxabuEi1h7ksmzoXgJT8jJoMR0xKYsJy_MpvtCRQ&e= > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Unless stated otherwise above: > > > > > > > > > > > IBM United Kingdom Limited - Registered in England and > > > Wales > > > > > with > > > > > > > > number > > > > > > > > > > > 741598. > > > > > > > > > > > Registered office: PO Box 41, North Harbour, Portsmouth, > > > > > Hampshire > > > > > > > > PO6 > > > > > > > > > > 3AU > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
