We have removed some public methods in the past, after a careful deprecation period, if they were not well working any more.
The sentiment I got from users is that careful cleanup is in fact appreciated, otherwise things get confusing over time (the deprecated methods cause noise in the API). Still, we need to be very careful here. I would suggest to - start with the non-public breaking methods - remove fold() (very long deprecated) - remove split() buggy Removing the env.socketStream() and env.fileStream() methods would probably be good, too. They are very long deprecated and they don't work well (with checkpoints) and the sources are the first thing a user needs to understand when starting with Flink, so removing noise there is super helpful. On Thu, Aug 20, 2020 at 8:53 AM Dawid Wysakowicz <dwysakow...@apache.org> wrote: > Hey Till, > > You've got a good point here. Removing some of the methods would cause > breaking the stability guarantees. I do understand if we decide not to > remove them for that reason, let me explain though why I am thinking it > might make sense to remove them already. First of all I am a bit afraid it > might take a long time before we arrive at the 2.0 version. We have not > ever discussed that in the community. At the same time a lot of the methods > already don't work or are buggy, and we do not fix them any more. > > Methods which removing would not break the Public guarantees: > > - ExecutionConfig#set/getCodeAnalysisMode (deprecated in 1.9) > - RuntimeContext#getAllAccumulators (deprecated in 0.10) > - ExecutionConfig#isLatencyTrackingEnabled (deprecated in 1.7) > - > StreamExecutionEnvironment#setNumberOfExecutionRetries/getNumberOfExecutionRetries > (not the equivalent in the ExecutionConfig) > - StreamExecutionEnvironment#setStateBackend(AbstractStateBackend) > (deprecated in 1.5) > > Methods which removing would break the Public guarantees: > > which have no effect: > > - ExecutionConfig#disable/enableSysoutLogging (deprecated in 1.10) > - ExecutionConfig#set/isFailTaskOnCheckpointError (deprecated in 1.9) > > which are buggy or discouraged and thus we do not support fixing them: > > - DataStream#split (deprecated in 1.8) > - DataStream#fold and all related classes and methods such as > FoldFunction, FoldingState, FoldingStateDescriptor ... (deprecated in > 1.3/1.4) > > The methods like: > > - > StreamExecutionEnvironment#readFile,readFileStream(...),socketTextStream(...),socketTextStream(...), > > - methods in (Connected)DataStream that specify keys as either indices > or field names > - > ExecutionConfig#setNumberOfExecutionRetries/getNumberOfExecutionRetries > > should be working just fine and I feel the least eager to remove those. > > I'd suggest I will open PRs for removing the methods that will not cause > breakage of the Public guarantees as the general feedback was rather > positive. For the rest I do understand the resentment to do so and will not > do it in the 1.x branch. Still I think it is valuable to have the > discussion. > > Best, > > Dawid > > > On 18/08/2020 09:26, Till Rohrmann wrote: > > Having looked at the proposed set of methods to remove I've noticed that > some of them are actually annotated with @Public. According to our > stability guarantees, only major releases (1.0, 2.0, etc.) can break APIs > with this annotation. Hence, I believe that we cannot simply remove them > unless the community decides to change the stability guarantees we give or > by making the next release a major release (Flink 2.0). > > Cheers, > Till > > On Tue, Aug 18, 2020 at 5:57 AM Yun Gao <yungao...@aliyun.com> wrote: > >> +1 for removing the methods that are deprecated for a while & have >> alternative methods. >> >> One specific thing is that if we remove the DataStream#split, do we >> consider enabling side-output in more operators in the future ? Currently >> it should be only available in ProcessFunctions, but not available to other >> commonly used UDF like Source or AsyncFunction[1]. >> >> One temporary solution occurs to me is to add a ProcessFunction after the >> operators want to use side-output. But I think the solution is not very >> direct to come up with and if it really works we might add it to the >> document of side-output. >> >> [1] https://issues.apache.org/jira/browse/FLINK-7954 >> >> Best, >> Yun >> >> ------------------Original Mail ------------------ >> *Sender:*Kostas Kloudas <kklou...@gmail.com> >> *Send Date:*Tue Aug 18 03:52:44 2020 >> *Recipients:*Dawid Wysakowicz <dwysakow...@apache.org> >> *CC:*dev <d...@flink.apache.org>, user <user@flink.apache.org> >> *Subject:*Re: [DISCUSS] Removing deprecated methods from DataStream API >> >>> +1 for removing them. >>> >>> >>> >>> From a quick look, most of them (not all) have been deprecated a long >>> time ago. >>> >>> >>> >>> Cheers, >>> >>> Kostas >>> >>> >>> >>> On Mon, Aug 17, 2020 at 9:37 PM Dawid Wysakowicz wrote: >>> >>> > >>> >>> > @David Yes, my idea was to remove any use of fold method and all >>> related classes including WindowedStream#fold >>> >>> > >>> >>> > @Klou Good idea to also remove the deprecated enableCheckpointing() & >>> StreamExecutionEnvironment#readFile and alike. I did another pass over some >>> of the classes and thought we could also drop: >>> >>> > >>> >>> > ExecutionConfig#set/getCodeAnalysisMode >>> >>> > ExecutionConfig#disable/enableSysoutLogging >>> >>> > ExecutionConfig#set/isFailTaskOnCheckpointError >>> >>> > ExecutionConfig#isLatencyTrackingEnabled >>> >>> > >>> >>> > As for the `forceCheckpointing` I am not fully convinced to doing it. >>> As far as I know iterations still do not participate in checkpointing >>> correctly. Therefore it still might make sense to force it. In other words >>> there is no real alternative to that method. Unless we only remove the >>> methods from StreamExecutionEnvironment and redirect to the setter in >>> CheckpointConfig. WDYT? >>> >>> > >>> >>> > An updated list of methods I suggest to remove: >>> >>> > >>> >>> > ExecutionConfig#set/getCodeAnalysisMode (deprecated in 1.9) >>> >>> > ExecutionConfig#disable/enableSysoutLogging (deprecated in 1.10) >>> >>> > ExecutionConfig#set/isFailTaskOnCheckpointError (deprecated in 1.9) >>> >>> > ExecutionConfig#isLatencyTrackingEnabled (deprecated in 1.7) >>> >>> > ExecutionConfig#(get)/setNumberOfExecutionRetries() (deprecated in >>> 1.1?) >>> >>> > >>> StreamExecutionEnvironment#readFile,readFileStream(...),socketTextStream(...),socketTextStream(...) >>> (deprecated in 1.2) >>> >>> > RuntimeContext#getAllAccumulators (deprecated in 0.10) >>> >>> > DataStream#fold and all related classes and methods such as >>> FoldFunction, FoldingState, FoldingStateDescriptor ... (deprecated in >>> 1.3/1.4) >>> >>> > StreamExecutionEnvironment#setStateBackend(AbstractStateBackend) >>> (deprecated in 1.5) >>> >>> > DataStream#split (deprecated in 1.8) >>> >>> > Methods in (Connected)DataStream that specify keys as either indices >>> or field names such as DataStream#keyBy, DataStream#partitionCustom, >>> ConnectedStream#keyBy, .... (deprecated in 1.11) >>> >>> > >>> >>> > Bear in mind that majority of the options listed above in >>> ExecutionConfig take no effect. They were left there purely to satisfy the >>> binary compatibility. Personally I don't see any benefit of leaving a >>> method and silently dropping the underlying feature. The only configuration >>> that is respected is setting the number of execution retries. >>> >>> > >>> >>> > I also wanted to make it explicit that most of the changes above would >>> result in a binary incompatibility and require additional exclusions in the >>> japicmp. Those are: >>> >>> > >>> >>> > ExecutionConfig#set/getCodeAnalysisMode (deprecated in 1.9) >>> >>> > ExecutionConfig#disable/enableSysoutLogging (deprecated in 1.10) >>> >>> > ExecutionConfig#set/isFailTaskOnCheckpointError (deprecated in 1.9) >>> >>> > ExecutionConfig#isLatencyTrackingEnabled (deprecated in 1.7) >>> >>> > ExecutionConfig#(get)/setNumberOfExecutionRetries() (deprecated in >>> 1.1?) >>> >>> > DataStream#fold and all related classes and methods such as >>> FoldFunction, FoldingState, FoldingStateDescriptor ... (deprecated in >>> 1.3/1.4) >>> >>> > DataStream#split (deprecated in 1.8) >>> >>> > Methods in (Connected)DataStream that specify keys as either indices >>> or field names such as DataStream#keyBy, DataStream#partitionCustom, >>> ConnectedStream#keyBy, .... (deprecated in 1.11) >>> >>> > >>> StreamExecutionEnvironment#readFile,readFileStream(...),socketTextStream(...),socketTextStream(...) >>> (deprecated in 1.2) >>> >>> > >>> >>> > Looking forward to more opinions on the issue. >>> >>> > >>> >>> > Best, >>> >>> > >>> >>> > Dawid >>> >>> > >>> >>> > >>> >>> > On 17/08/2020 12:49, Kostas Kloudas wrote: >>> >>> > >>> >>> > Thanks a lot for starting this Dawid, >>> >>> > >>> >>> > Big +1 for the proposed clean-up, and I would also add the deprecated >>> >>> > methods of the StreamExecutionEnvironment like: >>> >>> > >>> >>> > enableCheckpointing(long interval, CheckpointingMode mode, boolean >>> force) >>> >>> > enableCheckpointing() >>> >>> > isForceCheckpointing() >>> >>> > >>> >>> > readFile(FileInputFormat inputFormat,String >>> >>> > filePath,FileProcessingMode watchType,long interval, FilePathFilter >>> >>> > filter) >>> >>> > readFileStream(...) >>> >>> > >>> >>> > socketTextStream(String hostname, int port, char delimiter, long >>> maxRetry) >>> >>> > socketTextStream(String hostname, int port, char delimiter) >>> >>> > >>> >>> > There are more, like the (get)/setNumberOfExecutionRetries() that were >>> >>> > deprecated long ago, but I have not investigated to see if they are >>> >>> > actually easy to remove. >>> >>> > >>> >>> > Cheers, >>> >>> > Kostas >>> >>> > >>> >>> > On Mon, Aug 17, 2020 at 10:53 AM Dawid Wysakowicz >>> >>> > wrote: >>> >>> > >>> >>> > Hi devs and users, >>> >>> > >>> >>> > I wanted to ask you what do you think about removing some of the >>> deprecated APIs around the DataStream API. >>> >>> > >>> >>> > The APIs I have in mind are: >>> >>> > >>> >>> > RuntimeContext#getAllAccumulators (deprecated in 0.10) >>> >>> > DataStream#fold and all related classes and methods such as >>> FoldFunction, FoldingState, FoldingStateDescriptor ... (deprecated in >>> 1.3/1.4) >>> >>> > StreamExecutionEnvironment#setStateBackend(AbstractStateBackend) >>> (deprecated in 1.5) >>> >>> > DataStream#split (deprecated in 1.8) >>> >>> > Methods in (Connected)DataStream that specify keys as either indices >>> or field names such as DataStream#keyBy, DataStream#partitionCustom, >>> ConnectedStream#keyBy, .... (deprecated in 1.11) >>> >>> > >>> >>> > I think the first three should be straightforward. They are long >>> deprecated. The getAccumulators method is not used very often in my >>> opinion. The same applies to the DataStream#fold which additionally is not >>> very performant. Lastly the setStateBackend has an alternative with a class >>> from the AbstractStateBackend hierarchy, therefore it will be still code >>> compatible. Moreover if we remove the >>> #setStateBackend(AbstractStateBackend) we will get rid off warnings users >>> have right now when setting a statebackend as the correct method cannot be >>> used without an explicit casting. >>> >>> > >>> >>> > As for the DataStream#split I know there were some objections against >>> removing the #split method in the past. I still believe the output tags can >>> replace the split method already. >>> >>> > >>> >>> > The only problem in the last set of methods I propose to remove is >>> that they were deprecated only in the last release and those method were >>> only partially deprecated. Moreover some of the methods were not deprecated >>> in ConnectedStreams. Nevertheless I'd still be inclined to remove the >>> methods in this release. >>> >>> > >>> >>> > Let me know what do you think about it. >>> >>> > >>> >>> > Best, >>> >>> > >>> >>> > Dawid >> >>
