Awesome, thanks for looking into the Window improvements as well! (And I'm sorry I brought this upon you). I hope it's not too painful to get everything in the Windows ecosystem looking good and reasonable, and the benefits are pretty clear.
Haven't had a careful look through the POC yet but the proposal and public changes you described sound great. Thanks for the KIP! On Tue, Jul 28, 2020 at 10:27 AM John Roesler <vvcep...@apache.org> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > >
