In addition to fixing the annotation consistency formally, I think it is equally, if not more, important to see whether the public APIs we expose tell a good story. For example, if we say StreamConfig should be internal, some fair questions to ask is why our own AbstractStreamOperator needs it? Why does not a user-defined operator need it? Is there something in the StreamConfig we should expose as a public interface if not the entire class?
Couldn't agree more! That's the reason I am really hesitant to annotating more classes in AbstractStreamOperator with PublicEvolving, just because the class is marked that way.
Regarding the StreamTask / StreamConfig exposure, if you look at the StreamOperatorFactory which is also a PublicEvolving class, it actually exposes the StreamTask, StreamConfig as well as some other classes in the StreamOperatorParameters. So these classes are already exposed in multiple public APIs.AFAICT, the only reason why those are part of StreamOperatorParameters and StreamOperatorFactory is because AbstractStreamOperator needs them. Moreover those two classes are PublicEvolving because AbstractStreamOperator is.
Keeping our public API stability guarantee is really fundamental and critical to the users. With the current status of inconsistent API stability annotations, I don't see how can we assure of that. From what I can see, accidental backwards incompatible changes is likely going to keep happening. So I'd say let's see how to fix forward instead of doing nothing.I don't disagree with that. In fact I fully support that statement. I'm just trying to say that AbstractStreamOperator is not the abstraction we should make stable imo. As far as I am concerned it became "PublicEvolving" accidentally rather than it was well thought through. I had this idea in my head for some time now to rather extend the (Keyed)ProcessFunction interfaces with features that are missing there which make users implement operators directly. Those are pure interfaces, that should be much easier to handle in respect to API stability, as they do not contain any implementation just contract. In fact I truly believe abstract classes should never be part of any APIs.
Best, Dawid On 14/01/2023 00:45, Becket Qin wrote:
I don't have an overview of all the holes in our public API surface at the moment. It would be great if there's some tool to do a scan.In addition to fixing the annotation consistency formally, I think it is equally, if not more, important to see whether the public APIs we expose tell a good story. For example, if we say StreamConfig should be internal, some fair questions to ask is why our own AbstractStreamOperator needs it? Why does not a user-defined operator need it? Is there something in the StreamConfig we should expose as a public interface if not the entire class?Thanks, Jiangjie (Becket) QinOn Sat, Jan 14, 2023 at 5:36 AM Konstantin Knauf <kna...@apache.org> wrote:Hi Becket, > It is a basic rule of public API that anything exposed by a public interface should also be public. I agree with this in general. Did you get an overview of where we currently violate this? Is this something that the Arc42 architecture tests could test for so that as a first measure we don't introduce more occurrences (cc @Ingo). Maybe its justified to make a pass over all of these occurrences and resolve these occurrences one way or another either making the members/parameters @PublicEvoling or actually making a class/method @Internal even if its was @PublicEvoling before. I think, this could be the better option than having @PublicEvolving classes/methods that really aren't. Cheers, Konstantin Am Fr., 13. Jan. 2023 um 17:02 Uhr schrieb Becket Qin <becket....@gmail.com >: > Hi Dawid, > > Thanks for the reply. I am currently looking at the Beam Flink runner, and > there are quite some hacks the Beam runner has to do in order to deal with > the backwards incompatible changes in AbstractStreamOperator and some of > the classes exposed by it. Regardless of what we think, the fact is that > AbstractStreamOperator is marked as PublicEvolving today, and our users use > it. It is a basic rule of public API that anything exposed by a public > interface should also be public. This is the direct motivation of this > FLIP. > > Regarding the StreamTask / StreamConfig exposure, if you look at the > StreamOperatorFactory which is also a PublicEvolving class, it actually > exposes the StreamTask, StreamConfig as well as some other classes in the > StreamOperatorParameters. So these classes are already exposed in multiple > public APIs. > > Keeping our public API stability guarantee is really fundamental and > critical to the users. With the current status of inconsistent API > stability annotations, I don't see how can we assure of that. From what I > can see, accidental backwards incompatible changes is likely going to keep > happening. So I'd say let's see how to fix forward instead of doing > nothing. > > BTW, Initially I thought this is just an accidental mismatch, but after > further exam, it looks that it is a bigger issue. I guess one of the > reasons we end up in this situation is that we haven't really thought it > through regarding the boundary between framework and user space, i.e. what > framework primitives we want to expose to the users. So instead we just > expose a bunch of internal things and hope users only use the "stable" part > of them. > > Thanks, > > Jiangjie (Becket) Qin > > > On Fri, Jan 13, 2023 at 7:28 PM Dawid Wysakowicz <dwysakow...@apache.org> > wrote: > > > Hi Becket, > > > > May I ask what is the motivation for the change? > > > > I'm really skeptical about making any of those classes `Public` or > > `PublicEvolving`. As far as I am concerned there is no agreement in the > > community if StreamOperator is part of the `Public(Evolving)` API. At > > least I think it should not. I understand `AbstractStreamOperator` was > > marked with `PublicEvolving`, but I am really not convinced it was the > > right decision. > > > > The listed classes are not the only classes exposed to > > `AbstractStreamOperator` that are `Internal` that break the consistency > > and I am sure there is no question those should remain `Internal` such > > as e.g. StreamTask, StreamConfig, StreamingRuntimeContext and many more. > > > > As it stands I am strongly against giving any additional guarantees on > > API stability to the classes there unless there is a good motivation for > > a given feature and we're sure this is the best way to go forward. > > > > Thus I'm inclined to go with -1 on any proposal on changing annotations > > for the sake of matching the one on `AbstractStreamOperator`. I might be > > convinced to support requests to give better guarantees for well > > motivated features that are now internal though. > > > > Best, > > > > Dawid > > > > On 12/01/2023 10:20, Becket Qin wrote: > > > Hi flink devs, > > > > > > I'd like to start a discussion thread for FLIP-286[1]. > > > > > > As a recap, currently while AbstractStreamOperator is a class marked as > > > @PublicEvolving, some classes exposed via its methods / fields are > > > marked as @Internal. This FLIP wants to fix this inconsistency of > > > stability / scope annotation. > > > > > > Comments are welcome! > > > > > > Thanks, > > > > > > Jiangjie (Becket) Qin > > > > > > [1] > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=240880841 > > > > > >-- https://twitter.com/snntrablehttps://github.com/knaufk
OpenPGP_0x31D2DD10BFC15A2D.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
