Hi Jing, Thanks for your feedback!
> I am fine with @Internal and once we > use @Internal, we should add it to all classes within a module to keep the > consistency. I think the scope to keep consistency within a module is all interfaces, not all classes. For example, in changelog-statebackend module or flink-runtime/state/heap package (hashmap-statebackend), all interfaces are annotated, and classes' annotations are optional. The changes in this FLIP(Flip-420) also covers all interfaces of the rocksdb statebackend module. Best, Jinzhong On Wed, Mar 13, 2024 at 7:30 AM Jing Ge <j...@ververica.com.invalid> wrote: > 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. > > > > > > > > > > > > > > > > > > > > > > >
