So, what's next? It's my first KIP and I'm not familiar with all processes.
-Yuriy On Mon, Jul 6, 2020 at 1:32 AM John Roesler <vvcep...@apache.org> wrote: > Hi Yuriy, > > Thanks for the update! It looks good to me. > > Thanks, > John > > On Sun, Jul 5, 2020, at 03:27, Yuriy Badalyantc wrote: > > Hi John. > > > > I updated the KIP. An old proposed implementation is now in the rejected > > alternatives. > > > > - Yuriy > > > > On Sun, Jul 5, 2020 at 12:03 AM John Roesler <vvcep...@apache.org> > wrote: > > > > > Hi Yuriy, > > > > > > I agree, we can keep them separate. I just wanted to make you aware of > it. > > > > > > Thanks for the PR, it looks the way I expected. > > > > > > I just read over the KIP document again. I think it needs to be > updated to > > > the current proposal, and then we’ll be able to start the vote. > > > > > > Thanks, > > > John > > > > > > On Tue, Jun 30, 2020, at 04:58, Yuriy Badalyantc wrote: > > > > 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 > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > >
