Hey John, in the Public interface section, you mentioned `Do not add the new interface to JoinWindows, which should not be part of this hierarchy`, could you explain a bit why and what's the plan with JoinWindows?
On Tue, Jul 28, 2020 at 12:50 PM Sophie Blee-Goldman <sop...@confluent.io> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
