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