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
