Hi Sophie, A quick update: I've pushed a commit to the POC PR that includes the migration of Window to become a data class instead of an abstract class. It's quite a bit of code, but it does look like there is a clean deprecation/migration path.
The basic idea is that we drop the "abstract" modifier from Window an deprecate its constructor. We add a public static `Window.withBounds(start,end)` to replace the constructor. Because the constructor is deprecated, existing subclasses will continue to compile, but receive a deprecation warning. We'd also slightly modify EnumerableWindowDefinition to _not_ be a parameterized class and instead update windowsFor like this: <W extends Window> Map<Long, W> windowsFor(final long timestamp) Then, any existing caller that expects to get back a subclass of windows during the deprecation period would still get a valid return. For example, if you had a custom window definition, and you invoke this method in your tests, assigning it to a subclass of Window, all your code would still compile, but you would get deprecation warnings. After the deprecation period, we could migrate Window to be a final class with a private constructor safely. If that sounds reasonable to you, I can update the KIP accordingly. Thanks, -John On Mon, Jul 27, 2020, at 22:11, John Roesler wrote: > Thanks for the reply, Sophie. > > Yes, I'd neglected to specify that Windows will implement maxSize() > by delegating to size(). It's updated now. I'd also neglected to say that > I plan to alter both windowBy methods to use the new interface now. > Because Windows will implement the new interface, all implementations > will continue to work with windowBy. > So, yes, there are public methods changed, but no compatibility issues > arise. Existing implementations will only get a deprecation warning. > > The Window type is interesting. It actually seems to serve as just a data > container. It almost doesn't need to be subclassed at all, since all > implementations would just need to store the start and end bounds. > As far as I can tell, the only purpose to subclass it is to implement > "overlap(Window other)", to tell if the window is both the same type > as and overlaps with the other window. But weirdly, this is unused > in the codebase. > > So one way we could go is to try and migrate it to just a final class, > effectively a tuple of `(start, end)`. > > However, another opportunity is to let it be a witness of the actual type > of the window, so that you wouldn't be able to join a TimeWindow > with a SessionWindow, for example. > > However, because of covariance, it's more painful to change Window > than Windows, so it might not be worth it right now. If anything, it > would be more feasible to migrate toward the "simple data container" > approach. What are your thoughts? > > Thanks, > -John > > > On Mon, Jul 27, 2020, at 18:19, Sophie Blee-Goldman wrote: > > Thanks for taking the time to really fill in the background details for > > this KIP. > > The Motivation section is very informative. Hopefully this will also serve > > as a > > warning against making similar such mistakes in the future :P > > > > I notice that the `Window` class that > > parametrizes EnumerableWindowDefinition > > is also abstract. Did you consider migrating that to an interface as well? > > > > Also, pretty awesome that we can solve the issue with varying fixed sized > > windows > > (eg calendar months) on the side. For users who may have already extended > > Windows, > > do you plan to just have Windows implement the new #maxSize method and > > return the existing > > size until Windows gets removed? > > > > One final note: this seems to be implicitly implied throughout the KIP but > > just to be clear, > > you will be replacing any DSL methods that accept Windows with identical > > DSL methods > > that take the new EnumerableWindowDefinition as argument. So there is > > nothing deprecated > > and nothing added, but there are public methods changed. Is that right? > > > > On Sun, Jul 26, 2020 at 1:23 PM John Roesler <vvcep...@apache.org> wrote: > > > > > Thanks Sophie and Boyang for asking for specifics. > > > > > > As far as I can tell, if we were to _remove_ all the non-public-method > > > members from Windows, including any constructors, we are left with > > > effectively an interface. I think this was my plan before. I don't think > > > I realized at the time that it's possible to replace the entire class with > > > an interface. Now I realize it is possible, hence the motivation to do it. > > > > > > We can choose either to maintain forever the tech debt of having to > > > enforce that Windows look, sound, and act just like an interface, or we > > > can just replace it with an interface and be done with it. I.e., the > > > motivation is less maintenence burden for us and for users. > > > > > > Coincidentally, I had an interesting conversation with Matthias about > > > this interface, and he made me realize that "fixed size" isn't the > > > essential > > > nature of this entity. Instead being enumerable is. Reframing the > > > interface > > > in this way will enable us to implement variable sized windows like > > > MonthlyWindows. > > > > > > So, now there are two good reasons to vote for this KIP :) > > > > > > Anyway, I've updated the proposed interface and the motivation. > > > > > > To Sophie latter question, all of the necessary deprecation is listed > > > in the KIP. We do not have to deprecate any windowBy methods. > > > > > > Thanks, > > > -John > > > > > > On Sat, Jul 25, 2020, at 00:52, Boyang Chen wrote: > > > > Thanks for the KIP John. I share a similar concern with the motivation, > > > it > > > > would be good if you could cast light on the actual downside of using a > > > > base class vs the interface, is it making the code fragile, or requiring > > > > redundant implementation, etc. > > > > > > > > Boyang > > > > > > > > On Tue, Jul 21, 2020 at 2:19 PM Sophie Blee-Goldman <sop...@confluent.io > > > > > > > > wrote: > > > > > > > > > Hey John, > > > > > > > > > > Thanks for the KIP. I know this has been bugging you :) > > > > > > > > > > That said, I think the KIP is missing some elaboration in the > > > Motivation > > > > > section. > > > > > You mention a number of problems we've had and lived with in the past > > > -- > > > > > could > > > > > you give an example of one, and how it would be solved by your > > > proposal? > > > > > > > > > > By the way, I assume we would also need to deprecate all APIs that > > > accept a > > > > > Windows > > > > > parameter in favor of new ones that accept a > > > FixedSizeWindowDefinition? Off > > > > > the > > > > > top of my head that would be the windowedBy methods in KGroupedStream > > > and > > > > > CogroupedKStream > > > > > > > > > > On Tue, Jul 21, 2020 at 1:46 PM John Roesler <vvcep...@apache.org> > > > wrote: > > > > > > > > > > > Hello all, > > > > > > > > > > > > I'd like to propose KIP-645, to correct a small API mistake in > > > Streams. > > > > > > Fixing this now allows us to avoid perpetuating the mistake in new > > > work. > > > > > > For example, it will allow us to implement KIP-450 cleanly. > > > > > > > > > > > > The change itself should be seamless for users. > > > > > > > > > > > > Please see https://cwiki.apache.org/confluence/x/6SN4CQ for details. > > > > > > > > > > > > Thanks, > > > > > > -John > > > > > > > > > > > > > > > > > > > >
