Hi Jinzhong, Thanks for your clarification!
> As mentioned in Flink's “API compatibility guarantees”[1], "any API without > such an annotation is considered internal to Flink, with no guarantees > being provided." > I think these interfaces are treated as internal interfaces regardless of > whether they are marked @Internal. > > So, it's acceptable for me to either leave it unmarked or mark it as > @Internal. > It is a question of consistency. I am fine with @Internal and once we use @Internal, we should add it to all classes within a module to keep the consistency. Otherwise, when developers read some interfaces have @Internal, some other classes have no annotations. It turns out there are different meanings of with or without @Internal. But it is not a big deal. If no other one has the same concern. I will not block you :-) > As for FLIP-197, I found that the proposal in this flip has not actually > been implemented. Because FLINK-25352 [2] has not been resolved，it looks > like we can't add more info into the PublicEvolving annotation. > > Fair enough! Thanks for the hint! Best regards, Jing On Mon, Mar 11, 2024 at 9:03 AM Jinzhong Li <lijinzhong2...@gmail.com> wrote: > Hi Jing. > > Thanks for your suggestion. > > >> I was wondering if SingleStateIterator and RocksDBRestoreOperation are > exposed to users even if they are interfaces. > > I agree that very very few users implement these two interfaces, except for > some advanced users who implement their own StateBackend based on > rocksdbStateBackend. (The "StateBackend" and "EmbeddedRocksDBStateBackend" > are PublicEvolving interface) > > >> Only adding @Internal annotation to these two interfaces could be > considered as an implicit hint that users can replace the default behaviour > with their own implementation. > As mentioned in Flink's “API compatibility guarantees”[1], "any API without > such an annotation is considered internal to Flink, with no guarantees > being provided." > I think these interfaces are treated as internal interfaces regardless of > whether they are marked @Internal. > > So, it's acceptable for me to either leave it unmarked or mark it as > @Internal. > > >> Another suggestion is that we should start following FLIP-197[1](which > is already accepted by the community) to add more info into the > PublicEvolving annotation in order to kick off the graduation process. > > > As for FLIP-197, I found that the proposal in this flip has not actually > been implemented. Because FLINK-25352 [2] has not been resolved，it looks > like we can't add more info into the PublicEvolving annotation. > > > [1] > > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees > [2] https://issues.apache.org/jira/browse/FLINK-25352 > > Best, > Jinzhong > > > On Mon, Mar 11, 2024 at 12:29 AM Jing Ge <j...@ververica.com.invalid> > wrote: > > > Hi Jinzhong, > > > > Sorry for the late reply. The key guideline of adding visibility > annotation > > is whether the interface/class is a customer-facing API. I was wondering > if > > SingleStateIterator and RocksDBRestoreOperation are exposed to users even > > if they are interfaces, i.e. users can use their own implementation > > classes. The flink-statebackend-rocksdb module has many more classes than > > the FLIP described. Only adding @Internal annotation to these two > > interfaces could be considered as an implicit hint that users can replace > > the default behaviour with their own implementation. > > > > Another suggestion is that we should start following FLIP-197[1](which is > > already accepted by the community) to add more info into the > PublicEvolving > > annotation in order to kick off the graduation process. WDYT? > > > > Best regards, > > Jing > > > > [1] > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-197%3A+API+stability+graduation+process > > > > On Mon, Feb 26, 2024 at 2:22 PM Jinzhong Li <lijinzhong2...@gmail.com> > > wrote: > > > > > Hi all, > > > > > > Thanks for all the feedback. It seems there are no more questions > > > unaddressed. I would like to open the voting thread after three days. > > > > > > Please let me know if you have any concerns, thanks! > > > > > > Best, > > > Jinzhong Li > > > > > > On Mon, Feb 26, 2024 at 11:29 AM Yanfei Lei <fredia...@gmail.com> > wrote: > > > > > > > @Yun Tang > > > > Thanks for the information, +1 for marking > > > > `ConfigurableRocksDBOptionsFactory` as `PublicEvolving `. > > > > > > > > Best, > > > > Yanfei > > > > > > > > Yun Tang <myas...@live.com> 于2024年2月23日周五 19:54写道： > > > > > > > > > > Hi Jinzhong, > > > > > > > > > > Thanks for driving this topic, and +1 for fixing the lack of > > > annotation. > > > > > > > > > > @Yanfei the `ConfigurableRocksDBOptionsFactory` interface is > > introduced > > > > for user extension, you can refer to the doc[1], which shows an > example > > > of > > > > how to use this interface. > > > > > > > > > > > > > > > [1] > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/state/large_state_tuning/#tuning-rocksdb-memory > > > > > > > > > > > > > > > Best > > > > > Yun Tang > > > > > ________________________________ > > > > > From: Yanfei Lei <fredia...@gmail.com> > > > > > Sent: Thursday, February 22, 2024 15:39 > > > > > To: dev@flink.apache.org <dev@flink.apache.org> > > > > > Subject: Re: [DISCUSS]FLIP-420: Add API annotations for RocksDB > > > > StateBackend user-facing classes > > > > > > > > > > Hi Jinzhong, > > > > > Thanks for driving this! > > > > > > > > > > 1. I'm wondering if `ConfigurableRocksDBOptionsFactory` will be > used > > > > > by users, currently it looks like only developers use it in > rocksdb > > > > > state backend module. And Its only non-testing subclass > > > > > "DefaultConfigurableOptionsFactory" is marked @Deprecated. > > > > > 2. Regarding @Internal, according to the comments, it is used for > > > > > "Annotation to mark methods within stable, public APIs as an > internal > > > > > developer API." So marking "SingleStateIterator" and > > > > > "RocksDBRestoreOperation" as @Internal is acceptable for me. > > > > > > > > > > Best, > > > > > Yanfei > > > > > > > > > > Jinzhong Li <lijinzhong2...@gmail.com> 于2024年1月25日周四 12:16写道： > > > > > > > > > > > > Hi Zakelly, > > > > > > > > > > > > Thanks for your comments! > > > > > > > > > > > > 1）I agree that almost no user would use "RocksDBStateUploader" > and > > > > > > "RocksDBStateDownloader" to do something. It's fine for me to > keep > > > them > > > > > > unmarked. > > > > > > 2）Regarding "SingleStateIterator", I think it's acceptable to > > either > > > > leave > > > > > > it unmarked or mark it as @Internal. I just consider that > > > > > > SingleStateIterator is one interface with the "public" modifier > and > > > it > > > > is > > > > > > harmless to annotate it as @Internal. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Hangxiang, > > > > > > > > > > > > Thanks for the reminder! > > > > > > > > > > > > It makes sense to mark RocksDBStateBackendFactory as Deprecated. > > > > > > > > > > > > Best, > > > > > > Jinzhong Li > > > > > > > > > > > > > > > > > > On Thu, Jan 25, 2024 at 10:22 AM Hangxiang Yu < > master...@gmail.com > > > > > > > wrote: > > > > > > > > > > > > > Hi Jinzhong. > > > > > > > Thanks for driving this! > > > > > > > Some suggestions: > > > > > > > 1. As RocksDBStateBackend marked as Deprecated, We should also > > > > > > > mark RocksDBStateBackendFactory as Deprecated > > > > > > > 2. Since 1.19 will be freezed in 1.26. Let's adjust the target > > > > version to > > > > > > > 1.20 > > > > > > > > > > > > > > > > > > > > > On Wed, Jan 24, 2024 at 11:50 PM Zakelly Lan < > > > zakelly....@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Jinzhong, > > > > > > > > > > > > > > > > Thanks for driving this! +1 for fixing the lack of > annotation. > > > > > > > > > > > > > > > > I'm wondering if we really need to annotate > > > *RocksDBStateUploader* > > > > and > > > > > > > > *RocksDBStateDownloader > > > > > > > > *with @Internal, as they seem to be ordinary classes without > > > > interacting > > > > > > > > with other modules. > > > > > > > > Also, I have reservations about annotating > > *SingleStateIterator*, > > > > but I'd > > > > > > > > like to hear others' opinions and won't insist on this. > > > > > > > > > > > > > > > > Best, > > > > > > > > Zakelly > > > > > > > > > > > > > > > > On Wed, Jan 24, 2024 at 10:26 PM Jinzhong Li < > > > > lijinzhong2...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi devs, > > > > > > > > > > > > > > > > > > I’m opening this thread to discuss about FLIP-420: Add API > > > > annotations > > > > > > > > for > > > > > > > > > RocksDB StateBackend user-facing classes[1]. > > > > > > > > > > > > > > > > > > As described in FLINK-18255[2] , several user-facing > classes > > in > > > > > > > > > flink-statebackend-rocksdb module don't have any API > > > > annotations, not > > > > > > > > even > > > > > > > > > @PublicEvolving. This FLIP will add annotations for them to > > > > clarify > > > > > > > their > > > > > > > > > usage. > > > > > > > > > > > > > > > > > > Looking forward to hearing from you, thanks! > > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > Jinzhong Li > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-420%3A+Add+API+annotations+for+RocksDB+StateBackend+user-facing+classes > > > > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-18255 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Best, > > > > > > > Hangxiang. > > > > > > > > > > > > > > > > >
