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