Hi Becket, Speaking of tools, we have ArchUnit integrated in Flink. Extending the defined ArchRules [1] a little bit, you will get the wished scan result.
[1] https://github.com/apache/flink/blob/560b4612735a2b9cd3b5db88adf5cb223e85535b/flink-architecture-tests/flink-architecture-tests-production/src/main/java/org/apache/flink/architecture/rules/ApiAnnotationRules.java#L99 Best regards, Jing On Sat, Jan 14, 2023 at 12:46 AM Becket Qin <becket....@gmail.com> 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) 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 > > >
