I think the important part about the ConfigConstants is that the values don’t change. How they are represented inside of Flink, does not really matter. It would be good if that could be verified automatically.
Cheers, Till On Tue, Feb 16, 2016 at 2:59 PM, Robert Metzger <rmetz...@apache.org> wrote: > Thank you for taking care of this Fabian. > > I would like to bring your attention to the "ConfigConstants" class. I > marked it as "@Public". My intention is to ensure that we don't change > configuration parameters after the 1.0 release (this would break existing > configuration files of users). > > The tool I'm intending to use for ensuring interface stability does not > ensure that the values of the constants are not changed. It only ensures > that the variable names are not changed. What we have right now blocks us > from renaming the constants. > My reasoning was that if somebody is touching the constant's name, most > likely they'll also change the value. But that's just a poor best-effort > approach. > > What's your opinion? Should we remove the @Public annotation there? (With > the risk that configuration parameters might change between versions) or > leave it? > > > > On Mon, Feb 8, 2016 at 1:44 PM, Fabian Hueske <fhue...@gmail.com> wrote: > > > I created FLINK-3366 for renaming @Experimental to @PublicEvolving and > > FLINK-3367 to annotate all remaining API classes with @PublicEvolving. > > > > 2016-02-08 9:57 GMT+01:00 Maximilian Michels <m...@apache.org>: > > > > > Never thought about "Experimental" meaning unstable but I agree it > > > sounds better to use the term "Evolving". > > > > > > On Sun, Feb 7, 2016 at 12:45 AM, Fabian Hueske <fhue...@gmail.com> > > wrote: > > > > I agree, Experimental rather suggests unstable behavior than > > potentially > > > > changing interfaces. > > > > +1 for renaming to @PublicEvolving. > > > > > > > > Other opinions on strictly annotating all public interfaces with > > @Public > > > / > > > > @PublicEvolving and > > > > defaulting to @Internal for all remaining interfaces? > > > > > > > > 2016-02-06 15:27 GMT+01:00 Stephan Ewen <se...@apache.org>: > > > > > > > >> What "Experimental" is really saying is "Public, but not API > stable". > > In > > > >> that sense, "Internal" is not suitable, as it suggests that a method > > > will > > > >> never be public. > > > >> > > > >> What would you think of renaming "Experimental" to "PublicEvolving" > ? > > > >> > > > >> That would carry pretty much explicitly the meaning that it is > > intended > > > for > > > >> public use, the basic concept will stay, but it may change a bit > (code > > > >> using that may be adjusted a bit in the future). In contrast > > > "Experimental" > > > >> sounds to me like "no idea if that class/feature is going to be > there > > in > > > >> the future". > > > >> > > > >> What do you think? > > > >> > > > >> Stephan > > > >> > > > >> > > > >> On Sat, Feb 6, 2016 at 3:17 PM, Stephan Ewen <se...@apache.org> > > wrote: > > > >> > > > >> > Hi! > > > >> > > > > >> > These suggestions sound good in general. > > > >> > > > > >> > I am wondering if "Experimental" is not the wrong word here, > because > > > most > > > >> > of the things are not experimental, but just possibly subject to > > > slight > > > >> > changes (though API breaking). > > > >> > > > > >> > Experimental has the connotation that something is unstable > > (execution > > > >> > wise) and should best be avoided altogether. > > > >> > If half of the Flink code base is marked "Experimental", it seems > to > > > >> > contradict that this is actually in 1.0 shape. > > > >> > > > > >> > I actually like the term "Internal" that was also suggested. It > > pretty > > > >> > much says what we actually want to say: A certain method or class > is > > > not > > > >> > declared part of the public API, but as of now only internal API. > > > >> > > > > >> > Greetings, > > > >> > Stephan > > > >> > > > > >> > > > > >> > > > > >> > On Fri, Feb 5, 2016 at 8:38 PM, Fabian Hueske <fhue...@gmail.com> > > > wrote: > > > >> > > > > >> >> Hi, > > > >> >> > > > >> >> @Experimental and @Internal have the class scope, because we need > > to > > > be > > > >> >> able to mark internal classes of @Public classes as experimental > or > > > >> >> internal. > > > >> >> > > > >> >> In some cases we annotated classes as @Public and all (or most) > > > methods > > > >> as > > > >> >> @Experimental, to indicate that a class can be used, but its > > > internals > > > >> >> might change. > > > >> >> For example TypeInformation is a public class (and many classes > > that > > > >> >> extend > > > >> >> this class) but most methods are @Experimental to allow users to > > > used a > > > >> >> TypeInformation as is, but keep the freedom to change the > > internals. > > > If > > > >> >> you > > > >> >> find something that does not look correct, please start a > > discussion > > > >> >> about. > > > >> >> The annotations can still be changed before the 1.0 release. > > > >> >> > > > >> >> I agree with Max that annotating more classes with @Experimental > > (not > > > >> only > > > >> >> inner classes) would be a good idea. > > > >> >> IMO, we should annotate all classes that belong to the > user-facing > > > API > > > >> >> with > > > >> >> either @Public or @Experimental. This would mean that all > > > non-annotated > > > >> >> classes are not part of the API and must be considered as > > internal. I > > > >> >> believe this would avoid a lot of uncertainty and also easy the > > > >> annotation > > > >> >> management as a whole. For instance, we can more easily check how > > the > > > >> API > > > >> >> (stable and experimental) changed between releases. > > > >> >> > > > >> >> Cheers, Fabian > > > >> >> > > > >> >> 2016-02-05 17:24 GMT+01:00 Maximilian Michels <m...@apache.org>: > > > >> >> > > > >> >> > Hi Robert, > > > >> >> > > > > >> >> > Thanks a lot for all the work of going through the classes. At > > > first > > > >> >> > sight, the classes look quite well chosen. > > > >> >> > > > > >> >> > One question concerning the @Public, @Experimental, and > @Internal > > > >> >> > annotations: > > > >> >> > > > > >> >> > @Public may only be used for classes or interfaces. > @Experimental > > > or > > > >> >> > @Internal are used for marking methods in @Public classes only? > > If > > > >> >> > that is the case, we should restrict the annotations to this > > scope > > > via > > > >> >> > @Target. The current master also permits to tag classes with > > > >> >> > @Experimental or @Internal. > > > >> >> > > > > >> >> > Marking classes with @Experimental might actually make sense. > > What > > > was > > > >> >> > the rational behind always declaring a class public first to > > > restrict > > > >> >> > its methods to internal or experimental? > > > >> >> > > > > >> >> > Cheers, > > > >> >> > Max > > > >> >> > > > > >> >> > On Fri, Feb 5, 2016 at 3:07 PM, Robert Metzger < > > > rmetz...@apache.org> > > > >> >> > wrote: > > > >> >> > > Hi, > > > >> >> > > > > > >> >> > > tl;dr: we now have @Public, @Internal, @Experimental > > annotations. > > > >> >> Check > > > >> >> > > your stuff before the release! > > > >> >> > > > > > >> >> > > Fabian and I spend some time the last weeks to annotate all > > > classes > > > >> we > > > >> >> > > consider to be userfacing and stable with the "*@Public*" > > > >> annotation. > > > >> >> I > > > >> >> > > just pushed those changes to master. > > > >> >> > > > > > >> >> > > There is also an annotation "*@Internal*" for marking > > interfaces > > > >> users > > > >> >> > > should not use because they are internal to the system (for > > > example > > > >> >> > > "DataStream.getId()"). > > > >> >> > > > > > >> >> > > The annotation "*@Experimental*" marks unstable methods > within > > > >> stable > > > >> >> > > classes (for example SingleOutputStreamOperator.uid()). > > > >> >> > > Also note that we should also annotate @Deprecated methods > with > > > >> >> > > @Experimental so that they are not part of the stable > interface > > > >> (this > > > >> >> > works > > > >> >> > > only before the 1.0 release). I checked that all current > > > @Deprecated > > > >> >> > > annotations are excluded. > > > >> >> > > > > > >> >> > > > > > >> >> > > *I would like to ask everyone to spend some time before the > 1.0 > > > >> >> release > > > >> >> > to > > > >> >> > > go through some core classes and see if there is anything we > > > >> forgot.* > > > >> >> > > *We will not be able to touch methods and classes which are > > > public > > > >> >> after > > > >> >> > > the 1.0 release.* > > > >> >> > > > > > >> >> > > Some areas where I feel we should check closely are the > > > following: > > > >> >> > > - InputFormats > > > >> >> > > - State-related classes > > > >> >> > > - Windowing related classes > > > >> >> > > - DataStream API (global() / forward(); output to files; ... > ). > > > >> >> > > > > > >> >> > > Fabian and I also realized that there are some downsides to > > this > > > >> >> > annotation > > > >> >> > > approach. > > > >> >> > > a) By not annotating all classes, its easy to forget some > > > classes. > > > >> And > > > >> >> > its > > > >> >> > > not obvious to users that "no annotation" means "not public". > > > >> >> > > > > > >> >> > > b) For example the "SourceFunction.SourceContext" interface > is > > > >> @Public > > > >> >> > > because users use the methods of the interface. > > > >> >> > > However, the underlying implementations are internal to Flink > > > (users > > > >> >> will > > > >> >> > > most likely not implement their own SourceContexts). Adding a > > new > > > >> >> method > > > >> >> > to > > > >> >> > > the SourceContext interface will break its compatibilty > > (because > > > >> users > > > >> >> > > would need to implement the new method), however, for API > users > > > it > > > >> >> does > > > >> >> > not > > > >> >> > > matter when we are adding new methods. > > > >> >> > > We decided to make the interface @Public, but we added a > > comment > > > >> >> > explaining > > > >> >> > > the issue. > > > >> >> > > If we want to add a method after the 1.0 release, we can > define > > > an > > > >> >> > exclude > > > >> >> > > in the maven plugin which enforces the interface stability. > > > >> >> > > > > > >> >> > > > > > >> >> > > For making the whole annotation analysis business a bit > easier, > > > I'm > > > >> >> > > currently working on a little tool to output a list of public > > > >> classes > > > >> >> and > > > >> >> > > methods. I'll post that on the mailing list at a later point. > > > >> >> > > > > > >> >> > > Regards, > > > >> >> > > Robert > > > >> >> > > > > >> >> > > > >> > > > > >> > > > > >> > > > > > >
