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 > > > > > > > > > > > > > > > > > > > > >
