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