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