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) Qin On 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/snntrable > https://github.com/knaufk >
