Hi everybody! Looks like a discussion about KIP-513 could take a while. I think we should move forward with KIP-616 without waiting for KIP-513.
I created a new pull request for KIP-616: https://github.com/apache/kafka/pull/8955. It contains a new `org.apache.kafka.streams.scala.serialization.Serdes` object without name clash. An old one was marked as deprecated. This change is backward compatible and it could be merged in any further release. On Wed, Jun 3, 2020 at 12:41 PM Yuriy Badalyantc <lmne...@gmail.com> wrote: > Hi, John > > Thanks for pointing that out. I expressed my thoughts about KIP-513 and > its connection to KIP-616 in the KIP-513 mail list. > > - Yuriy > > On Sun, May 31, 2020 at 1:26 AM John Roesler <vvcep...@apache.org> wrote: > >> Hi Yuriy, >> >> I was just looking back at KIP-513, and I’m wondering if there’s any >> overlap we should consider here, or if they are just orthogonal. >> >> Thanks, >> -John >> >> On Thu, May 28, 2020, at 21:36, Yuriy Badalyantc wrote: >> > At the current moment, I think John's plan is better than the original >> plan >> > described in the KIP. I think we should create a new `Serdes` in another >> > package. The old one will be deprecated. >> > >> > - Yuriy >> > >> > On Fri, May 29, 2020 at 8:58 AM John Roesler <vvcep...@apache.org> >> wrote: >> > >> > > Thanks, Matthias, >> > > >> > > If we go with the approach Yuriy and I agreed on, to deprecate and >> replace >> > > the whole class and not just a few of the methods, then the timeline >> is >> > > less of a concern. Under that plan, Yuriy can just write the new class >> > > exactly the way he wants and people can cleanly swap over to the new >> > > pattern when they are ready. >> > > >> > > The timeline was more significant if we were just going to deprecate >> some >> > > methods and add new methods to the existing class. That plan requires >> two >> > > implementation phases, where we first deprecate the existing methods >> and >> > > later swap the implicits at the same time we remove the deprecated >> members. >> > > Aside from the complexity of that approach, it’s not a breakage free >> path, >> > > as some users would be forced to continue using the deprecated members >> > > until a future release drops them, breaking their source code, and >> only >> > > then can they update their code. >> > > >> > > That wouldn’t be the end of the world, and we’ve had to do the same >> thing >> > > in the past with the implicit conversations, but this is a much wider >> > > scope, since it’s all the serdes. I’m happy with the new plan, since >> it’s >> > > not only one step, but also it provides everyone a breakage-free path. >> > > >> > > We can still consider dropping the deprecated class in 3.0; I just >> wanted >> > > to clarify how the timeline issue has changed. >> > > >> > > Thanks, >> > > John >> > > >> > > On Thu, May 28, 2020, at 20:34, Matthias J. Sax wrote: >> > > > I am not a Scale person, so I cannot really contribute much. >> However for >> > > > the deprecation period, if we get the change into 2.7, it might be >> ok to >> > > > remove the deprecated classed in 3.0. >> > > > >> > > > It would only be one minor release in between what is a little bit >> short >> > > > (we usually prefer at least two minor released, better three), but >> if we >> > > > have a good reason for it, it might be ok. >> > > > >> > > > If we cannot remove it in 3.0, it seems there would be a 4.0 in >> about a >> > > > year(?) when ZK removal is finished and we can remove the deprecated >> > > > code than. >> > > > >> > > > >> > > > -Matthias >> > > > >> > > > On 5/28/20 7:39 AM, John Roesler wrote: >> > > > > Hi Yuriy, >> > > > > >> > > > > Sounds good to me! I had a feeling we were bringing different >> context >> > > > > to the discussion; thanks for sticking with the conversation >> until we >> > > got >> > > > > it hashed out. >> > > > > >> > > > > I'm glad you prefer Serde*s*, since having multiple different >> classes >> > > with >> > > > > the same name leads to all kinds of trouble. "Serdes" seems >> relatively >> > > > > safe because people in the Scala lib won't be using the Java >> Serdes >> > > class, >> > > > > and they won't be using the deprecated and non-deprecated one at >> the >> > > > > same time. >> > > > > >> > > > > Thank again, >> > > > > -John >> > > > > >> > > > > On Thu, May 28, 2020, at 02:21, Yuriy Badalyantc wrote: >> > > > >> Ok, I understood you, John. I wasn't sure about kafka deprecation >> > > policy >> > > > >> and thought that the full cycle could be done with 2.7 version. >> > > Waiting for >> > > > >> 3.0 is too much, I agree with it. >> > > > >> >> > > > >> So, I think creating one more `Serdes` in another package is our >> way. >> > > I >> > > > >> suggest one of the following: >> > > > >> 1. `org.apache.kafka.streams.scala.serde.Serdes` >> > > > >> 2. `org.apache.kafka.streams.scala.serialization.Serdes` >> > > > >> >> > > > >> About `Serde` vs `Serdes`. I'm strongly against `Serde` because >> it >> > > would >> > > > >> lead to a new name clash with the >> > > > >> `org.apache.kafka.common.serialization.Serde`. >> > > > >> >> > > > >> - Yuriy >> > > > >> >> > > > >> On Thu, May 28, 2020 at 11:12 AM John Roesler < >> vvcep...@apache.org> >> > > wrote: >> > > > >> >> > > > >>> Hi Yuriy, >> > > > >>> >> > > > >>> Thanks for the clarification. >> > > > >>> >> > > > >>> I guess my concern is twofold: >> > > > >>> 1. We typically leave deprecated methods in place for at least a >> > > major >> > > > >>> release cycle before removing them, so it would seem abrupt to >> have a >> > > > >>> deprecation period of only one minor release. If we follow the >> same >> > > pattern >> > > > >>> here, it would take over a year to finish this KIP. >> > > > >>> 2. It doesn’t seem like there is a nonbreaking deprecation path >> at >> > > all if >> > > > >>> people enumerate their imports (if they don’t use a wildcard). >> In >> > > that >> > > > >>> case, they would have no path to implicitly use the newly named >> > > serdes, and >> > > > >>> therefore they would have no way to avoid continuing to use the >> > > deprecated >> > > > >>> ones. >> > > > >>> >> > > > >>> Since you mentioned that your reason is mainly the preference >> for >> > > the name >> > > > >>> “Serde” or “Serdes”, can we explore just using one of those? >> Would >> > > it cause >> > > > >>> some kind of conflict to use >> org.apache.kafka.streams.scala.Serde or >> > > to use >> > > > >>> Serdes in a different package, like >> > > > >>> org.apache.kafka.streams.scala.implicit.Serdes? >> > > > >>> >> > > > >>> I empathize with this desire. I faced the same dilemma when I >> wanted >> > > to >> > > > >>> replace Processor but keep the class name in KIP-478. I wound up >> > > creating a >> > > > >>> new package for the new Processor. >> > > > >>> >> > > > >>> Thanks, >> > > > >>> John >> > > > >>> >> > > > >>> On Wed, May 27, 2020, at 22:20, Yuriy Badalyantc wrote: >> > > > >>>> Hi John, >> > > > >>>> >> > > > >>>> I'm stick with the `org.apache.kafka.streams.scala.Serdes` >> because >> > > it's >> > > > >>>> sort of conventional in the scala community. If you have a >> typeclass >> > > > >>> `Foo`, >> > > > >>>> you probably will search `Foo` related stuff in the `Foo` or >> maybe >> > > `Foos` >> > > > >>>> (plural). All other places are far less discoverable for the >> > > developers. >> > > > >>>> >> > > > >>>> I agree that the migration path is a bit complex for such >> change. >> > > But I >> > > > >>>> think it's more important to provide good developer experience >> than >> > > to >> > > > >>>> simplify migration. Also, I think it's debatable which >> migration >> > > path is >> > > > >>>> better for library users. If we would create, for example, >> > > `Serdes2`, >> > > > >>>> library users will have to modify their code if they used any >> part >> > > of the >> > > > >>>> old `Serde`. With my approach, most of the old code will still >> work >> > > > >>> without >> > > > >>>> changes. Only explicit usage of implicits will need to be fixed >> > > (because >> > > > >>>> names will be changed, and old names will be deprecated). >> Wildcard >> > > > >>> imports >> > > > >>>> will work without changes and will not lead to a name clash. >> > > Moreover, >> > > > >>> many >> > > > >>>> users may not notice name clash problems. And with my migration >> > > path, >> > > > >>> they >> > > > >>>> will not notice any changes at all. >> > > > >>>> >> > > > >>>> - Yuriy >> > > > >>>> >> > > > >>>> On Thu, May 28, 2020 at 7:48 AM John Roesler < >> vvcep...@apache.org> >> > > > >>> wrote: >> > > > >>>> >> > > > >>>>> Hi Yuriy, >> > > > >>>>> >> > > > >>>>> Thanks for the reply. I guess I've been out of the Scala game >> for a >> > > > >>>>> while; all this summoner business is totally new to me. >> > > > >>>>> >> > > > >>>>> I think I followed the rationale you provided, but I still >> don't >> > > see >> > > > >>>>> why you can't implement your whole plan in a new class. What >> > > > >>>>> is special about the existing Serdes class? >> > > > >>>>> >> > > > >>>>> Thanks, >> > > > >>>>> -John >> > > > >>>>> >> > > > >>>>> On Tue, May 19, 2020, at 01:18, Yuriy Badalyantc wrote: >> > > > >>>>>> Hi John, >> > > > >>>>>> >> > > > >>>>>> Your suggestion looks interesting. I think it's technically >> > > doable. >> > > > >>> But >> > > > >>>>> I'm >> > > > >>>>>> not sure that this is the better solution. I will try to >> explain. >> > > > >>> From >> > > > >>>>> the >> > > > >>>>>> scala developers' perspective, `Serde` looks really like a >> > > typeclass. >> > > > >>>>>> Typical typeclass in pure scala will look like this: >> > > > >>>>>> >> > > > >>>>>> ``` >> > > > >>>>>> trait Serde[A] { >> > > > >>>>>> def serialize(data: A): Array[Byte] >> > > > >>>>>> def deserialize(data: Array[Byte]): A >> > > > >>>>>> } >> > > > >>>>>> object Serde extends DefaultSerdes { >> > > > >>>>>> // "summoner" function. With this I can write `Serde[A]` >> and >> > > this >> > > > >>> serde >> > > > >>>>>> will be implicitly summonned. >> > > > >>>>>> def apply[A](implicit ev: Serde[A]): Serde[A] = ev >> > > > >>>>>> } >> > > > >>>>>> >> > > > >>>>>> trait DefaultSerdes { >> > > > >>>>>> // default instances here >> > > > >>>>>> } >> > > > >>>>>> ``` >> > > > >>>>>> >> > > > >>>>>> Usage example (note, that there are no wildcards imports >> here): >> > > > >>>>>> ``` >> > > > >>>>>> object Main extends App { >> > > > >>>>>> import Serde // not wildcard import >> > > > >>>>>> >> > > > >>>>>> // explicit summonning: >> > > > >>>>>> val stringSerde = Serde[String] // using summoner >> > > > >>>>>> stringSerde.serialize(???) >> > > > >>>>>> >> > > > >>>>>> // implicit summonning >> > > > >>>>>> def serialize[A: Serde](a: A) = { >> > > > >>>>>> Serde[A].serialize(a) // summoner again >> > > > >>>>>> } >> > > > >>>>>> serialize("foo") >> > > > >>>>>> } >> > > > >>>>>> ``` >> > > > >>>>>> >> > > > >>>>>> Examples are pretty silly, but I just want to show common >> > > patterns of >> > > > >>>>>> working with typeclasses in scala. All default instances in >> the >> > > usage >> > > > >>>>>> examples are found using implicits searching mechanism. Scala >> > > > >>> compiler >> > > > >>>>>> searches implicits in a lot of places. Including companion >> > > objects. >> > > > >>> In my >> > > > >>>>>> examples compiler will found `Serde[String]` instance in the >> > > > >>> companion >> > > > >>>>>> object of `Serde` typeclass. >> > > > >>>>>> >> > > > >>>>>> Also, I want to pay attention to the summoner function. It >> makes >> > > > >>> usage of >> > > > >>>>>> typeclasses very neat and clear. >> > > > >>>>>> >> > > > >>>>>> The example above was the example of the perfect solution >> for the >> > > > >>> scala >> > > > >>>>>> developers. But this solution requires to create separate >> `Serde` >> > > > >>>>>> typeclass, to make all this implicit searching stuff works. I >> > > don't >> > > > >>> think >> > > > >>>>>> that it worth it, because a lot of code should be >> reimplemented >> > > using >> > > > >>>>> this >> > > > >>>>>> new typeclass. But the main point of my example is to show >> the >> > > > >>> perfect >> > > > >>>>>> solution. And I think we should strive to provide developer >> > > > >>> experience >> > > > >>>>>> close to this. >> > > > >>>>>> >> > > > >>>>>> It's a bit out of the scope of my KIP, but I have a plan to >> make >> > > > >>>>>> `org.apache.kafka.streams.scala.Serdes` more closer to the >> > > solution >> > > > >>>>> above. >> > > > >>>>>> It could be done in 2 steps: >> > > > >>>>>> 1. Fix implicit names. >> > > > >>>>>> 2. Add summoner function. >> > > > >>>>>> >> > > > >>>>>> And with this scala developers will be able to write almost >> the >> > > same >> > > > >>> code >> > > > >>>>>> as in the example above: >> > > > >>>>>> ``` >> > > > >>>>>> object Main extends App { >> > > > >>>>>> import org.apache.kafka.streams.scala.Serdes // not >> wildcard >> > > import >> > > > >>>>>> >> > > > >>>>>> val stringSerde = Serdes[String] >> > > > >>>>>> stringSerde.serialize(???) >> > > > >>>>>> >> > > > >>>>>> def serialize[A: Serde](a: A) = { >> > > > >>>>>> Serdes[A].serialize(a) >> > > > >>>>>> } >> > > > >>>>>> serialize("foo") >> > > > >>>>>> } >> > > > >>>>>> ``` >> > > > >>>>>> Of course, wildcard import will still work. >> > > > >>>>>> >> > > > >>>>>> Other names will make this new entity (containing default >> > > implicits) >> > > > >>> less >> > > > >>>>>> discoverable. And summoner usage, in this case, will look >> weird: >> > > > >>>>>> ``` >> > > > >>>>>> object Main extends App { >> > > > >>>>>> import org.apache.kafka.streams.scala.DefaultSerdes // not >> > > wildcard >> > > > >>>>> import >> > > > >>>>>> >> > > > >>>>>> val stringSerde = DefaultSerdes[String] >> > > > >>>>>> stringSerde.serialize(???) >> > > > >>>>>> >> > > > >>>>>> def serialize[A: Serde](a: A) = { >> > > > >>>>>> DefaultSerdes[A].serialize(a) >> > > > >>>>>> } >> > > > >>>>>> serialize("foo") >> > > > >>>>>> } >> > > > >>>>>> ``` >> > > > >>>>>> >> > > > >>>>>> So, I think it's more important to provide a solid and >> familiar >> > > > >>> developer >> > > > >>>>>> experience for the scala developer. And renaming (or >> creating a >> > > new >> > > > >>>>>> version) of `Serdes` will not help here. >> > > > >>>>>> >> > > > >>>>>> -Yuriy >> > > > >>>>>> >> > > > >>>>>> On Tue, May 19, 2020 at 11:56 AM John Roesler < >> > > vvcep...@apache.org> >> > > > >>>>> wrote: >> > > > >>>>>> >> > > > >>>>>>> Hi Yuriy, >> > > > >>>>>>> >> > > > >>>>>>> Thanks so much for the KIP! I didn’t anticipate the problem >> you >> > > > >>> laid >> > > > >>>>> out >> > > > >>>>>>> in the KIP, but I find it very plausible. >> > > > >>>>>>> >> > > > >>>>>>> Thanks for pushing back on the “convention” and raising the >> > > issue, >> > > > >>> and >> > > > >>>>>>> also volunteering a solution! >> > > > >>>>>>> >> > > > >>>>>>> I’m wondering if we can “fix” it in one shot by just >> deprecating >> > > > >>> the >> > > > >>>>> whole >> > > > >>>>>>> Serdes class and replacing it with a new one containing the >> defs >> > > > >>> you >> > > > >>>>>>> proposed. Then, people could just switch their import to >> the new >> > > > >>> one. >> > > > >>>>>>> >> > > > >>>>>>> Of course the new class needs to have a different name, >> which is >> > > > >>>>> always a >> > > > >>>>>>> challenge in situations like this, so I might just throw out >> > > > >>>>> ImplicitSerdes >> > > > >>>>>>> as an option. >> > > > >>>>>>> >> > > > >>>>>>> Do you think this would work? >> > > > >>>>>>> >> > > > >>>>>>> Thanks again, >> > > > >>>>>>> John >> > > > >>>>>>> >> > > > >>>>>>> On Mon, May 18, 2020, at 23:35, Yuriy Badalyantc wrote: >> > > > >>>>>>>> Hi, >> > > > >>>>>>>> >> > > > >>>>>>>> I would like to propose KIP-616 to fix naming clash in the >> kafka >> > > > >>>>>>>> streams scala API: >> > > > >>>>>>>> >> > > > >>>>>>> >> > > > >>>>> >> > > > >>> >> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-616%3A+Rename+implicit+Serdes+instances+in+kafka-streams-scala >> > > > >>>>>>>> >> > > > >>>>>>>> Looking forward to your feedback. >> > > > >>>>>>>> >> > > > >>>>>>>> -Yuriy >> > > > >>>>>>>> >> > > > >>>>>>> >> > > > >>>>>> >> > > > >>>>> >> > > > >>>> >> > > > >>> >> > > > >> >> > > > >> > > > >> > > > Attachments: >> > > > * signature.asc >> > > >> > >> >
