Hi Jane, Hi Jack, Thanks for sharing your thoughts. It makes sense. Did you mean literally all classes in table-api-java, table-api-java-bridge, and table-common modules should have at least one visibility annotations? Any other modules or only these three modules?
Agree, we should add ArchUnit rules to ensure it has to be done. Same things should be propagated to DataStream API (modules) later. Best regards, Jing On Mon, Jul 31, 2023 at 8:10 PM Jark Wu <imj...@gmail.com> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
