Hi Mickael, Thanks for the updated KIP.
1) I don’t feel too strong about this. I understand your point of view. 2) I just looked at the updated interface and I find a little weird to already have a deprecated method in a new interface, don’t you? If we believe that using Configurable is a good thing, it may be better to revise our position regarding 1) and get rid of that deprecated init method. Otherwise, it may be better to not implement Configurable after all. I leave this up to you. 3) Thanks for the clarification. Best, David Le jeu. 11 juin 2020 à 16:34, Mickael Maison <mickael.mai...@gmail.com> a écrit : > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
