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
