Hi Mickael, Thanks for your answer. Understood.
+1 (non-binding) Best, David On Thu, Jun 18, 2020 at 10:15 AM Mickael Maison <mickael.mai...@gmail.com> wrote: > Hi David, > > Thanks again for your feedback! > > 2) I think it makes sense to use Configurable to make this interface > consistent with all the other interfaces Kafka exposes. Yes it's > unfortunate it introduces a deprecated method in the new interface but > I don't think it's a big deal. It allows users to keep their existing > code and just recompile it. They'll see deprecation warnings and will > have time to move to the new interface. We will also remove the > deprecated parts, including that method, in the next major release. > > On Thu, Jun 11, 2020 at 9:07 PM David Jacot <dja...@confluent.io> wrote: > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
