Thanks for the updates. LGTM now. Hi Jing,
In my opinion, enforcing visibility annotations for all classes in the API module has a major benefit of simplifying CI checks for missing annotations. Best, Jark On Mon, 31 Jul 2023 at 12:33, liu ron <ron9....@gmail.com> wrote: > Hi, Jane > > Thanks for driving this discussion, I think this would be very useful for > developer, +1. > > Best, > Ron > > Jane Chan <qingyue....@gmail.com> 于2023年7月31日周一 10:57写道： > > > Hi Jing, > > > > According to your proposal, does it mean that there will be no public > class > > > that has no annotation? > > > > > > Yes, every class should be marked with the visibility annotation, but > this > > is restricted only to the classes in the table-api-java, > > table-api-java-bridge, and table-common modules, as they are exposed to > > users. > > > > > > In the end, every public class must have one annotation and all classes > > > with @Internal will have the same meaning for developers as they had no > > > annotation before. Developers will ignore the annotation and continue > > > depending on them. > > > > > > > The Java doc for `@Internal` clearly states that it is internal to Flink. > > Suppose users continue to use it and encounter compatibility issues in > > subsequent versions, we can respond that it is because they are using an > > internal class that should not be relied on. However, if a class is > > unmarked, it is undefined and could be either `@PublicEvolving` or > > `@Internal`, resulting in higher interpretation costs. If that's the > case, > > we'd better explain the purpose of these classes to users from the > > beginning. IMO this is necessary unless someday we can extract away all > the > > classes that users should not rely upon from these three modules. > > > > Another point is that if we allow classes without visibility annotations, > > Flink developers may easily forget to mark some APIs that should have > been > > marked as `@PublicEvolving` during development. This phenomenon can also > be > > seen from the sheet I've collected. Moreover, it is difficult to check > this > > situation through the automated rules. > > > > Best, > > Jane > > > > On Sat, Jul 29, 2023 at 10:59 PM Jing Ge <j...@ververica.com.invalid> > > wrote: > > > > > Hi Jane, > > > > > > Thanks for the clarification. Commonly speaking, it is a good idea to > > show > > > the clear information that a class is only used internally, i.e. please > > > don't rely on it. However, the rule of using @Internal in the community > > is > > > not clearly defined, at least it is not as clear as > > > @Public/@PublicEvolving/@Experimental > > > are. According to your proposal, does it mean that there will be no > > public > > > class that has no annotation? In the end, every public class must have > > one > > > annotation and all classes with @Internal will have the same meaning > for > > > developers as they had no annotation before. Developers will ignore > > > the annotation and continue depending on them. It looks like a typical > > > involution: we will come back to the position where we were but with > > > extra @Internal maintenance effort. > > > > > > Best regards, > > > Jing > > > > > > On Sat, Jul 29, 2023 at 3:01 PM Jane Chan <qingyue....@gmail.com> > wrote: > > > > > > > Hi all, > > > > > > > > Thanks for your valuable feedback. Here are my thoughts. > > > > > > > > To Jark > > > > I agree with your suggestions, and I've updated the sheet > accordingly. > > > > > > > > To Lincoln > > > > > > > > > For the `DynamicFilteringEvent`, tend to keep it 'internal' since > > it's > > > a > > > > > concreate implementation of `SourceEvent`(and other two > implementers > > > are > > > > > not public ones) . > > > > > > > > > > > > I have checked the corresponding FLIP [1], and according to the > design > > > doc, > > > > it suggested being marked as `@PublicEvolving`. At the same time, the > > > class > > > > `DynamicFilteringData`, which was introduced with it, was also marked > > as > > > > `@PublicEvolving`. I think we can cc Gen Luo <luogen...@gmail.com> > to > > > help > > > > clarify. > > > > > > > > > > > > For the `LookupOptions` and `Trigger`s, because they're all in the > > public > > > > > interfaces of the flip[2], I'm fine with making them all public > ones > > or > > > > > just excluding the Trigger implemantors, cc @Qingsheng can you also > > > help > > > > to > > > > > check this? > > > > > > > > > > > > > I'm fine with both. Let's wait for Qingsheng's opinion. > > > > > > > > > > > > For the `BuiltInFunctionDefinitions$Builder`, I think it should be > > > > > `BuiltInFunctionDefinition$Builder` > > > > > > > > > > > > Yes, this is a typo, and I've corrected it. > > > > > > > > > > > > To Jing > > > > > > > > do we really need to > > > > > mark some many classes as @Internal? What is the exactly different > > > > between > > > > > a public class with no annotation and with the @Internal? > > > > > > > > > > > > IMO it is still necessary. From a user's perspective, marking a class > > as > > > > @Internal has a clear directionality, indicating that this is an > > internal > > > > class, and I should not rely on it. However, facing an unmarked > class, > > I > > > > wonder whether it is safe to depend on it in my code. From a > > developer's > > > > perspective, marking a class as @Internal also helps us to be more > > > > confident when iterating and updating interfaces. We can be sure that > > > this > > > > proposed change will not have unexpected behavior (because we have > > > informed > > > > users that it is internal and cannot promise the same compatibility > > > > guarantee as public APIs). > > > > > > > > [1] > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-248%3A+Introduce+dynamic+partition+pruning#:~:text=PublicEvolving%0Apublic%20class-,DynamicFilteringEvent,-implements%20SourceEvent%20%7B > > > > > > > > Best, > > > > Jane > > > > > > > > On Wed, Jul 26, 2023 at 1:26 AM Jing Ge <j...@ververica.com.invalid> > > > > wrote: > > > > > > > > > Hi Jane, > > > > > > > > > > Thanks for your effort of walking through all classes and compiling > > the > > > > > sheet. It is quite helpful. Just out of curiosity, do we really > need > > to > > > > > mark some many classes as @Internal? What is the exactly different > > > > between > > > > > a public class with no annotation and with the @Internal? > > > > > > > > > > Best regards, > > > > > Jing > > > > > > > > > > > > > > > On Tue, Jul 25, 2023 at 11:06 AM Lincoln Lee < > lincoln.8...@gmail.com > > > > > > > > wrote: > > > > > > > > > > > Hi Jane, > > > > > > > > > > > > Thanks for driving this! Overall the proposed annotations looks > > good > > > to > > > > > me. > > > > > > Some comments for the table[1]: > > > > > > > > > > > > For the `DynamicFilteringEvent`, tend to keep it 'internal' since > > > it's > > > > a > > > > > > concreate implementation of `SourceEvent`(and other two > > implementers > > > > are > > > > > > not public ones) . > > > > > > > > > > > > For the `LookupOptions` and `Trigger`s, because they're all in > the > > > > public > > > > > > interfaces of the flip[2], I'm fine with making them all public > > ones > > > or > > > > > > just excluding the Trigger implemantors, cc @Qingsheng can you > also > > > > help > > > > > to > > > > > > check this? > > > > > > > > > > > > For the `BuiltInFunctionDefinitions$Builder`, I think it should > be > > > > > > `BuiltInFunctionDefinition$Builder`. > > > > > > > > > > > > > > > > > > [1]. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/spreadsheets/d/1e8M0tUtKkZXEd8rCZtZ0C6Ty9QkNaPySsrCgz0vEID4/edit?usp=sharing > > > > > > [2]. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-221%3A+Abstraction+for+lookup+source+cache+and+metric#FLIP221:Abstractionforlookupsourcecacheandmetric-PublicInterfaces > > > > > > > > > > > > Best, > > > > > > Lincoln Lee > > > > > > > > > > > > Jark Wu <imj...@gmail.com> 于2023年7月25日周二 10:47写道： > > > > > > > > > > > > > Hi Jane, > > > > > > > > > > > > > > Thanks for kicking off this work and collecting the detailed > > list. > > > > > > > > > > > > > > +1 to add the missing annotation. > > > > > > > > > > > > > > This often confuses me whether the class can be modified > without > > > > > breaking > > > > > > > the compatibility > > > > > > > when looking at classes in table-common and table-api. > > Explicitly > > > > mark > > > > > > the > > > > > > > visibility can be > > > > > > > helpful in this case. > > > > > > > > > > > > > > I have some additional suggestions wrt the class annotations: > > > > > > > - classes in org.apache.flink.table.catalog.stats.* can be > > > > > > @PublicEvolving, > > > > > > > because all the classes in there are needed to build the > > > > > @PublicEvolving > > > > > > > CatalogColumnStatistics. > > > > > > > - PeriodicCacheReloadTrigger and TimedCacheReloadTrigger can be > > > > > > > @PublicEvolving, > > > > > > > they are built-in implementations of cache reload trigger and > are > > > > > exposed > > > > > > > to connectors. > > > > > > > - CoreModule can be @PublicEvolving to allow users use it in > > > > > > > TableEnv#loadModule(name, Module). > > > > > > > > > > > > > > Best, > > > > > > > Jark > > > > > > > > > > > > > > > > > > > > > On Fri, 21 Jul 2023 at 00:34, Jane Chan <qingyue....@gmail.com > > > > > > wrote: > > > > > > > > > > > > > > > Hi, Devs, > > > > > > > > > > > > > > > > I would like to start a discussion on adding missing > visibility > > > > > > > annotation > > > > > > > > (PublicEvolving / Internal etc.) for Table APIs. > > > > > > > > > > > > > > > > The motivation for starting this discussion was during the > > > cleanup > > > > of > > > > > > > which > > > > > > > > Table API to be deprecated for version 2.0, I noticed that > some > > > of > > > > > the > > > > > > > APIs > > > > > > > > lack visibility annotations, which may lead to users relying > on > > > > APIs > > > > > > that > > > > > > > > should have been marked as internal. > > > > > > > > > > > > > > > > Therefore, I have compiled a sheet[1] listing the currently > > > > unmarked > > > > > > > > classes under the table-api-java, table-api-java-bridge, and > > > > > > table-common > > > > > > > > modules and the recommended annotations to be added. > > > > > > > > > > > > > > > > My thought is that all public classes/interfaces within the > > three > > > > > > modules > > > > > > > > mentioned above should be explicitly marked, and we might > > > consider > > > > > > > > introducing a new architectural rule to perform auto-check on > > > newly > > > > > > added > > > > > > > > APIs in the future. > > > > > > > > > > > > > > > > Let me explain the details. > > > > > > > > > > > > > > > > 1. Why table-api-java, table-api-java-bridge, and > table-common? > > > > > > > > Because according to Flink's application project > configuration > > > > > doc[2], > > > > > > > > table-api-java and table-api-java-bridge are the leading > > > > dependencies > > > > > > for > > > > > > > > users to develop a table program. Although flink-table-common > > is > > > > not > > > > > > > > listed, it is the core dependency for users to implement a > > > > > User-Defined > > > > > > > > Function/Connector[3]. > > > > > > > > > > > > > > > > 2. How are the classes listed in this table compiled? > > > > > > > > I use a customized Intellij profile to perform the code > > > inspection > > > > > > under > > > > > > > > these three modules to find all public classes/interfaces > > without > > > > API > > > > > > > > visibility annotations, along with a manual check. > > > > > > > > > > > > > > > > 3. How is the suggested API visibility to be determined? > > > > > > > > For all APIs suggested as PublicEvolving, I added a comment > on > > > the > > > > > cell > > > > > > > to > > > > > > > > explain the reason. The rest APIs, which are indicated as > > > Internal, > > > > > are > > > > > > > > either util classes or implementations. > > > > > > > > > > > > > > > > I'm looking forward to your ideas, and it would be great if > any > > > > > > > interested > > > > > > > > developers could help review this list together. > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/spreadsheets/d/1e8M0tUtKkZXEd8rCZtZ0C6Ty9QkNaPySsrCgz0vEID4/edit?usp=sharing > > > > > > > > [2] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/dev/configuration/overview/#which-dependencies-do-you-need > > > > > > > > [3] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/sourcessinks/#project-configuration > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > Jane > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
