Hi Becket, Thanks a lot, I have no problem any more. And I have made further modifications to FLIP-389[1]. In summary, this flip has 2 goals:
- Annotate SingleThreadFetcherManager as PublicEvolving. - Shield FutureCompletingBlockingQueue from users and limit all operations on FutureCompletingBlockingQueue in SplitFetcherManager. All the changes are listed below: - Mark constructor of SourceReaderBase and SingleThreadMultiplexSourceReaderBase as @Depricated and provide a new constructor without FutureCompletingBlockingQueue. - Mark SplitFetcherManager andSingleThreadFetcherManager as `@PublicEvolving`, mark constructor of SplitFetcherManager and SingleThreadFetcherManager as @Depricated and provide a new constructor without FutureCompletingBlockingQueue. - SplitFetcherManager provides wrapper methods for FutureCompletingBlockingQueue to replace its usage in SourceReaderBase. - Mark SplitFetcher and SplitFetcherTask as PublicEvolving. Any additional questions regarding this FLIP? Looking forward to hearing from you. Thanks, Hongshun Wang [1] https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=278465498 On Wed, Nov 22, 2023 at 10:15 AM Becket Qin <becket....@gmail.com> wrote: > Hi Hongshun, > > The constructor of the SplitFetcher is already package private. So it can > only be accessed from the classes in the package > org.apache.flink.connector.base.source.reader.fetcher. And apparently, user > classes should not be in this package. Therefore, even if we mark the > SplitFetcher class as PublicEvolving, the constructor is not available to > the users. Only the public and protected methods are considered public API > in this case. Private / package private methods and fields are still > internal. > > Thanks, > > Jiangjie (Becket) Qin > > On Wed, Nov 22, 2023 at 9:46 AM Hongshun Wang <loserwang1...@gmail.com> > wrote: > > > Hi Becket, > > > > If SplitFetcherManager becomes PublicEvolving, that also means > SplitFetcher > > > needs to be PublicEvolving, because it is returned by the protected > > method > > > SplitFetcherManager.createSplitFetcher(). > > > > > > > > > it looks like there is no need to expose the constructor of > SplitFetcher > > > to the end users. Having an interface of SplitFetcher is also fine, but > > > might not be necessary in this case. > > > > > > > > I don't know how to make SplitFetcher as PublicEnvolving but not to > expose > > the constructor of SplitFetcher to the end users? > > > > Thanks, > > Hongshun Wang > > > > On Tue, Nov 21, 2023 at 7:23 PM Becket Qin <becket....@gmail.com> wrote: > > > > > Hi Hongshun, > > > > > > Do we need to expose the constructor of SplitFetcher to the users? > > Ideally, > > > users should always get a new fetcher instance by calling > > > SplitFetcherManager.createSplitFetcher(). Or, they can get an existing > > > SplitFetcher by looking up in the SplitFetcherManager.fetchers map. I > > think > > > this makes sense because a SplitFetcher should always belong to a > > > SplitFetcherManager. Therefore, it should be created via a > > > SplitFetcherManager as well. So, it looks like there is no need to > expose > > > the constructor of SplitFetcher to the end users. > > > > > > Having an interface of SplitFetcher is also fine, but might not be > > > necessary in this case. > > > > > > Thanks, > > > > > > Jiangjie (Becket) Qin > > > > > > On Tue, Nov 21, 2023 at 10:36 AM Hongshun Wang < > loserwang1...@gmail.com> > > > wrote: > > > > > > > Hi Becket, > > > > > > > > > Additionally, SplitFetcherTask requires > FutureCompletingBlockingQueue > > > as > > > > a constructor parameter, which is not allowed now. > > > > Sorry, it was my writing mistake. What I meant is that *SplitFetcher* > > > > requires FutureCompletingBlockingQueue as a constructor parameter. > > > > SplitFetcher > > > > is a class rather than Interface. Therefore, I want to change > > > > SplitFetcher to a public Interface and moving its implementation > > > > details to an implement > > > > subclass . > > > > > > > > Thanks, > > > > Hongshun Wang > > > > > > > > On Fri, Nov 17, 2023 at 6:21 PM Becket Qin <becket....@gmail.com> > > wrote: > > > > > > > > > Hi Hongshun, > > > > > > > > > > SplitFetcher.enqueueTask() returns void, right? SplitFetcherTask is > > > > already > > > > > an interface, and we need to make that as a PublicEvolving API as > > well. > > > > > > > > > > So overall, a source developer can potentially do a few things in > the > > > > > SplitFetcherManager. > > > > > 1. for customized logic including split-to-fetcher assignment, > > > threading > > > > > model, etc. > > > > > 2. create their own SplitFetcherTask for the SplitFetcher / > > SplitReader > > > > to > > > > > execute in a coordinated manner. > > > > > > > > > > It should be powerful enough for the vast majority of the source > > > > > implementation, if not all. > > > > > > > > > > > > > > > Additionally, SplitFetcherTask requires > FutureCompletingBlockingQueue > > > > > > as a > > > > > > constructor parameter, which is not allowed > > > > > > now. > > > > > > > > > > Are you referring to FetchTask which implements SplitFetcherTask? > > That > > > > > class will remain internal. > > > > > > > > > > Thanks, > > > > > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > On Fri, Nov 17, 2023 at 5:23 PM Hongshun Wang < > > loserwang1...@gmail.com > > > > > > > > > wrote: > > > > > > > > > > > Hi, Jiangjie(Becket) , > > > > > > Thank you for your advice. I have learned a lot. > > > > > > > > > > > > If SplitFetcherManager becomes PublicEvolving, that also means > > > > > > > SplitFetcher needs to be PublicEvolving, because it is returned > > by > > > > the > > > > > > > protected method SplitFetcherManager.createSplitFetcher(). > > > > > > > > > > > > I completely agree with you. However, if SplitFetcher becomes > > > > > > PublicEvolving, SplitFetcherTask also needs to be PublicEvolving > > > > > > because it is returned by the public method > > SplitFetcher#enqueueTask. > > > > > > Additionally, SplitFetcherTask requires > > FutureCompletingBlockingQueue > > > > > > as a > > > > > > constructor parameter, which is not allowed > > > > > > now. Therefore, I propose changing SplitFetcher to a public > > Interface > > > > > > and moving its implementation details to an implement class > (e.g., > > > > > > SplitFetcherImpl or another suitable name). SplitFetcherImpl will > > be > > > > > > marked as internal and managed by SplitFetcherManager, > > > > > > and put data in the queue. > > > > > > Subclasses of SplitFetcherManager can only use the SplitFetcher > > > > > interface, > > > > > > also ensuring that the current subclasses are not affected. > > > > > > > > > > > > > > > > > > > > > > > > The current SplitFetcherManager basically looks up > > > > > > > the SplitT from the fetcher with the split Id, and immediately > > > passes > > > > > the > > > > > > > SplitT back to the fetcher, which is unnecessary. > > > > > > > > > > > > I inferred that this is because SplitReader#pauseOrResumeSplits > > > > > > requires SplitT instead of SpiltId. Perhaps some external source > > > > > > requires more information to pause. However, SplitReader doesn't > > > store > > > > > > all its split data, while SplitFetcherManager saves them. > > > > > > CC, @Dawid Wysakowicz > > > > > > > > > > > > > > > > > > > > > > > > If not, SplitFetcher.pause() and > > > > > > > SplitFetcher.resume() can be removed. In fact, they seem no > > longer > > > > used > > > > > > > anywhere. > > > > > > > > > > > > It seems no use any more. CC, @Arvid Heise > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > Hongshun Wang > > > > > > > > > > > > On Fri, Nov 17, 2023 at 11:42 AM Becket Qin < > becket....@gmail.com> > > > > > wrote: > > > > > > > > > > > > > Hi Hongshun, > > > > > > > > > > > > > > Thanks for updating the FLIP. I think that makes sense. A few > > > > comments > > > > > > > below: > > > > > > > > > > > > > > 1. If SplitFetcherManager becomes PublicEvolving, that also > means > > > > > > > SplitFetcher needs to be PublicEvolving, because it is returned > > by > > > > the > > > > > > > protected method SplitFetcherManager.createSplitFetcher(). > > > > > > > > > > > > > > 2. When checking the API of the classes to be marked as > > > > PublicEvolving, > > > > > > > there might be a few methods' signatures worth some discussion. > > > > > > > > > > > > > > For SplitFetcherManager: > > > > > > > a) Currently removeSplits() methods takes a list of SplitT. I > am > > > > > > wondering > > > > > > > if it should be a list of splitIds. SplitT actually contains > two > > > > parts > > > > > of > > > > > > > information, the static split Id and some dynamically changing > > > state > > > > of > > > > > > the > > > > > > > split (e.g. Kafka consumer offset). The source of truth for the > > > > dynamic > > > > > > > state is SourceReaderBase. Currently we are passing in the full > > > > source > > > > > > > split with the dynamic state for split removal. But it looks > like > > > > only > > > > > > > split id is needed for the split removal. > > > > > > > Maybe this is intentional, as sometimes when a SplitReader > > removes > > > a > > > > > > split, > > > > > > > it also wants to know the dynamic state of the split. If so, we > > can > > > > > keep > > > > > > it > > > > > > > as is. But then the question is why > > > > > > > SplitFetcherManager.pauseAndResumeSplits() only takes split ids > > > > instead > > > > > > of > > > > > > > SplitT. Should we make them consistent? > > > > > > > > > > > > > > For SplitFetcher: > > > > > > > a) The SplitFetcher.pauseOrResumeSplits() method takes > > collections > > > of > > > > > > > SplitT as arguments. We may want to adjust that according to > what > > > we > > > > do > > > > > > to > > > > > > > the SplitFetcherManager. The current SplitFetcherManager > > basically > > > > > looks > > > > > > up > > > > > > > the SplitT from the fetcher with the split Id, and immediately > > > passes > > > > > the > > > > > > > SplitT back to the fetcher, which is unnecessary. > > > > > > > b) After supporting split level pause and resume, do we still > > need > > > > > split > > > > > > > fetcher level pause and resume? If not, SplitFetcher.pause() > and > > > > > > > SplitFetcher.resume() can be removed. In fact, they seem no > > longer > > > > used > > > > > > > anywhere. > > > > > > > > > > > > > > Other than the above potential API adjustment before we mark > the > > > > > classes > > > > > > > PublicEvolving, the API looks fine to me. > > > > > > > > > > > > > > I think it is good timing for deprecation now. We will mark the > > > > > impacted > > > > > > > constructors as deprecated in 1.19, and remove them in release > of > > > > 2.0. > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Nov 16, 2023 at 8:26 PM Hongshun Wang < > > > > loserwang1...@gmail.com > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Devs, > > > > > > > > > > > > > > > > I have just modified the content of FLIP-389: Annotate > > > > > > > > SingleThreadFetcherManager as PublicEvolving[1]. > > > > > > > > > > > > > > > > Now this Flip mainly do two thing: > > > > > > > > > > > > > > > > 1. Annotate SingleThreadFetcherManager as PublicEvolving > > > > > > > > 2. Remove all public constructors which use > > > > > > > > FutureCompletingBlockingQueue. This will make many > > > constructors > > > > as > > > > > > > > @Depricated. > > > > > > > > > > > > > > > > This may influence many connectors, so I am looking forward > to > > > > > hearing > > > > > > > from > > > > > > > > you. > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > Hongshun > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=278465498 > > > > > > > > > > > > > > > > On Wed, Nov 15, 2023 at 7:57 AM Becket Qin < > > becket....@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Hongshun, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > However, it will be tricky because SplitFetcherManager > > > includes > > > > > <E, > > > > > > > > > SplitT > > > > > > > > > > extends SourceSplit>, while FutureCompletingBlockingQueue > > > > > includes > > > > > > > <T>. > > > > > > > > > > This means that SplitFetcherManager would have to be > > modified > > > > to > > > > > > <T, > > > > > > > E, > > > > > > > > > > SplitT extends SourceSplit>, which would affect the > > > > compatibility > > > > > > of > > > > > > > > the > > > > > > > > > > SplitFetcherManager class. I'm afraid this change will > > > > influence > > > > > > > other > > > > > > > > > > sources. > > > > > > > > > > > > > > > > > > Although the FutureCompletingBlockingQueue class itself > has a > > > > > > template > > > > > > > > > class <T>. In the SourceReaderBase and SplitFetcherManager, > > > this > > > > > <T> > > > > > > is > > > > > > > > > actually RecordsWithSplitIds<E>. So it looks like we can > just > > > let > > > > > > > > > SplitFetcherManager.poll() return a RecordsWithSplitIds<E>. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > > > > > > > > > On Tue, Nov 14, 2023 at 8:11 PM Hongshun Wang < > > > > > > loserwang1...@gmail.com > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Becket, > > > > > > > > > > I agree with you and try to modify this Flip[1], > > which > > > > > > include > > > > > > > > > these > > > > > > > > > > changes: > > > > > > > > > > > > > > > > > > > > 1. Mark constructor of > > > SingleThreadMultiplexSourceReaderBase > > > > > as > > > > > > > > > > @Depricated > > > > > > > > > > 2. > > > > > > > > > > > > > > > > > > > > Mark constructor of SourceReaderBase as *@Depricated* > > and > > > > > > provide > > > > > > > a > > > > > > > > > new > > > > > > > > > > constructor without > > > > > > > > > > > > > > > > > > > > FutureCompletingBlockingQueue > > > > > > > > > > 3. > > > > > > > > > > > > > > > > > > > > Mark constructor of SplitFetcherManager > > > > > > > > andSingleThreadFetcherManager > > > > > > > > > > as *@Depricated* and provide a new constructor > > > > > > > > > > without FutureCompletingBlockingQueue. Mark > > > > > SplitFetcherManager > > > > > > > > > > andSingleThreadFetcherManager as *@PublicEvolving* > > > > > > > > > > 4. > > > > > > > > > > > > > > > > > > > > SplitFetcherManager provides wrapper methods for > > > > > > > > > > FutureCompletingBlockingQueue to replace its usage in > > > > > > > > > SourceReaderBase. > > > > > > > > > > Then we can use FutureCompletingBlockingQueue only in > > > > > > > > > > SplitFetcherManager. > > > > > > > > > > > > > > > > > > > > However, it will be tricky because SplitFetcherManager > > > includes > > > > > <E, > > > > > > > > > SplitT > > > > > > > > > > extends SourceSplit>, while FutureCompletingBlockingQueue > > > > > includes > > > > > > > <T>. > > > > > > > > > > This means that SplitFetcherManager would have to be > > modified > > > > to > > > > > > <T, > > > > > > > E, > > > > > > > > > > SplitT extends SourceSplit>, which would affect the > > > > compatibility > > > > > > of > > > > > > > > the > > > > > > > > > > SplitFetcherManager class. I'm afraid this change will > > > > influence > > > > > > > other > > > > > > > > > > sources. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Looking forward to hearing from you. > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > Hongshun > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=278465498 > > > > > > > > > > > > > > > > > > > > On Sat, Nov 11, 2023 at 10:55 AM Becket Qin < > > > > > becket....@gmail.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Hongshun and Martijn, > > > > > > > > > > > > > > > > > > > > > > Sorry for the late reply as I was on travel and still > > > > catching > > > > > up > > > > > > > > with > > > > > > > > > > the > > > > > > > > > > > emails. Please allow me to provide more context. > > > > > > > > > > > > > > > > > > > > > > 1. The original design of SplitFetcherManager and its > > > > > subclasses > > > > > > > was > > > > > > > > to > > > > > > > > > > > make them public to the Source developers. The goal is > to > > > let > > > > > us > > > > > > > take > > > > > > > > > > care > > > > > > > > > > > of the threading model, while the Source developers can > > > just > > > > > > focus > > > > > > > on > > > > > > > > > the > > > > > > > > > > > SplitReader implementation. Therefore, I think making > > > > > > > > > > SplitFetcherManater / > > > > > > > > > > > SingleThreadFetcherManager public aligns with the > > original > > > > > > design. > > > > > > > > That > > > > > > > > > > is > > > > > > > > > > > also why these classes are exposed in the constructor > of > > > > > > > > > > SourceReaderBase. > > > > > > > > > > > > > > > > > > > > > > 2. For FutureCompletingBlockingQueue, as a hindsight, > it > > > > might > > > > > be > > > > > > > > > better > > > > > > > > > > to > > > > > > > > > > > not expose it to the Source developers. They are > unlikely > > > to > > > > > use > > > > > > it > > > > > > > > > > > anywhere other than just constructing it. The reason > that > > > > > > > > > > > FutureCompletingBlockingQueue is currently exposed in > the > > > > > > > > > > SourceReaderBase > > > > > > > > > > > constructor is because both the SplitFetcherManager and > > > > > > > > > SourceReaderBase > > > > > > > > > > > need it. One way to hide the > > FutureCompletingBlockingQueue > > > > from > > > > > > the > > > > > > > > > > public > > > > > > > > > > > API is to make SplitFetcherManager the only owner class > > of > > > > the > > > > > > > queue, > > > > > > > > > and > > > > > > > > > > > expose some of its methods via SplitFetcherManager. > This > > > way, > > > > > the > > > > > > > > > > > SourceReaderBase can invoke the methods via > > > > > SplitFetcherManager. > > > > > > I > > > > > > > > > > believe > > > > > > > > > > > this also makes the code slightly cleaner. > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > > > > > > > > > > > > > On Fri, Nov 10, 2023 at 12:28 PM Hongshun Wang < > > > > > > > > > loserwang1...@gmail.com> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > @Martijn, I agree with you. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I also have two questions at the beginning: > > > > > > > > > > > > > > > > > > > > > > > > - Why is an Internal class > > > > > > > > > > > > exposed as a constructor param of a Public class? > > > > > > > > > > > > - Should these classes be exposed as public? > > > > > > > > > > > > > > > > > > > > > > > > For the first question, I noticed that before the > > > original > > > > > > > > Jira[1] , > > > > > > > > > > > > all these classes missed the annotate , so it was not > > > > > abnormal > > > > > > > that > > > > > > > > > > > > FutureCompletingBlockingQueue and > > > > SingleThreadFetcherManager > > > > > > were > > > > > > > > > > > > constructor params of > > > > SingleThreadMultiplexSourceReaderBase. > > > > > > > > > > > > However, > > > > > > > > > > > > this jira marked FutureCompletingBlockingQueue and > > > > > > > > > > > > SingleThreadFetcherManager as Internal, while marked > > > > > > > > > > > > SingleThreadMultiplexSourceReaderBase as Public. > It's a > > > > good > > > > > > > > choice, > > > > > > > > > > > > but also forget that FutureCompletingBlockingQueue > and > > > > > > > > > > > > SingleThreadFetcherManager have already been exposed > > by > > > > > > > > > > > > SingleThreadMultiplexSourceReaderBase. > > > > > > > > > > > > Thus, this problem occurs because we didn't > > > > > > > > > > > > clearly define the boundaries at the origin design. > We > > > > should > > > > > > pay > > > > > > > > > more > > > > > > > > > > > > attention to it when creating a new class. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > For the second question, I think at least > > > > SplitFetcherManager > > > > > > > > > > > > should be Public. There are few reasons: > > > > > > > > > > > > > > > > > > > > > > > > - Connector developers want to decide their own > > > > > > > > > > > > thread mode. For example, Whether to recycle > > fetchers > > > by > > > > > > > > > overriding > > > > > > > > > > > > SplitFetcherManager#maybeShutdownFinishedFetchers > > > > > > > > > > > > when idle. Sometimes, developers want > > > > SplitFetcherManager > > > > > > > react > > > > > > > > > as a > > > > > > > > > > > > FixedThreadPool, because > > > > > > > > > > > > each time a thread is recycled then recreated, the > > > > context > > > > > > > > > > > > resources need to be rebuilt. I met a related issue > in > > > > flink > > > > > > > > cdc[2]. > > > > > > > > > > > > - > > > > > > > > > > > > KafkaSourceFetcherManager[3] also extends > > > > > > > > > > > > SingleThreadFetcherManager to commitOffsets. But now > > > kafka > > > > > > souce > > > > > > > is > > > > > > > > > > > > not in Flink repository, so it's not allowed any > more. > > > > > > > > > > > > > > > > > > > > > > > > [1] > https://issues.apache.org/jira/browse/FLINK-22358 > > > > > > > > > > > > > > > > > > > > > > > > [2] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/ververica/flink-cdc-connectors/pull/2571#issuecomment-1797585418 > > > > > > > > > > > > > > > > > > > > > > > > [3] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/flink-connector-kafka/blob/979791c4c71e944c16c51419cf9a84aa1f8fea4c/flink-connector-kafka/src/main/java/org/apache/flink/connector/kafka/source/reader/fetcher/KafkaSourceFetcherManager.java#L52 > > > > > > > > > > > > > > > > > > > > > > > > Looking forward to hearing from you. > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > Hongshun > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Nov 9, 2023 at 11:46 PM Martijn Visser < > > > > > > > > > > martijnvis...@apache.org > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > > > > > I'm looking at the original Jira that introduced > > these > > > > > > > stability > > > > > > > > > > > > > designations [1] and I'm just curious if it was > > > intended > > > > > that > > > > > > > > these > > > > > > > > > > > > > Internal classes would be used directly, or if we > > just > > > > > > haven't > > > > > > > > > > created > > > > > > > > > > > > > the right abstractions? The reason for asking is > > > because > > > > > > moving > > > > > > > > > > > > > something from Internal to a public designation is > an > > > > easy > > > > > > fix, > > > > > > > > > but I > > > > > > > > > > > > > want to make sure that it's also the right fix. If > we > > > are > > > > > > > missing > > > > > > > > > > good > > > > > > > > > > > > > abstractions, then I would rather invest in those. > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > > > > > > > > > > > Martijn > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > https://issues.apache.org/jira/browse/FLINK-22358 > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 8, 2023 at 12:40 PM Leonard Xu < > > > > > > xbjt...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks Hongshun for starting this discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > +1 from my side. > > > > > > > > > > > > > > > > > > > > > > > > > > > > IIRC, @Jiangjie(Becket) also mentioned this in > > > > > FLINK-31324 > > > > > > > > > > > comment[1]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > Leonard > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://issues.apache.org/jira/browse/FLINK-31324?focusedCommentId=17696756&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17696756 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2023年11月8日 下午5:42，Hongshun Wang < > > > > > loserwang1...@gmail.com > > > > > > > > > > > > > > > 写道： > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi devs, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I would like to start a discussion on FLIP-389: > > > > > Annotate > > > > > > > > > > > > > > > SingleThreadFetcherManager and > > > > > > > FutureCompletingBlockingQueue > > > > > > > > as > > > > > > > > > > > > > > > PublicEvolving.[ > > > > > > > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=278465498 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Though the SingleThreadFetcherManager is > > annotated > > > as > > > > > > > > Internal, > > > > > > > > > > it > > > > > > > > > > > > > actually > > > > > > > > > > > > > > > acts as some-degree public API, which is widely > > > used > > > > in > > > > > > > many > > > > > > > > > > > > connector > > > > > > > > > > > > > > > projects: flink-cdc-connector > > > > > > > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/ververica/flink-cdc-connectors/blob/release-2.3.0/flink-connector-mysql-cdc/src/main/java/com/ververica/cdc/connectors/mysql/source/reader/MySqlSourceReader.java#L93 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > , flink-connector-mongodb > > > > > > > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/flink-connector-mongodb/blob/main/flink-connector-mongodb/src/main/java/org/apache/flink/connector/mongodb/source/reader/MongoSourceReader.java#L58 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > soon. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Moreover, even the constructor of > > > > > > > > > > > > SingleThreadMultiplexSourceReaderBase > > > > > > > > > > > > > > > (which is PublicEvolving) includes the params > of > > > > > > > > > > > > > SingleThreadFetcherManager > > > > > > > > > > > > > > > and FutureCompletingBlockingQueue. That means > > that > > > > the > > > > > > > > > > > > > > > SingleThreadFetcherManager and > > > > > > > FutureCompletingBlockingQueue > > > > > > > > > > have > > > > > > > > > > > > > already > > > > > > > > > > > > > > > been exposed to users for a long time and are > > > widely > > > > > > used. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Considering that all source implementations are > > > using > > > > > > them > > > > > > > de > > > > > > > > > > > facto, > > > > > > > > > > > > > why > > > > > > > > > > > > > > > not annotate SingleThreadFetcherManager and > > > > > > > > > > > > > FutureCompletingBlockingQueue > > > > > > > > > > > > > > > as PublicEvolving so that developers will > modify > > it > > > > > more > > > > > > > > > > carefully > > > > > > > > > > > to > > > > > > > > > > > > > avoid > > > > > > > > > > > > > > > any potential issues. As shown in > > FLINK-31324[2], > > > > > > > > > FLINK-28853[3] > > > > > > > > > > > > used > > > > > > > > > > > > > > > to change the default constructor of > > > > > > > > > SingleThreadFetcherManager. > > > > > > > > > > > > > However, > > > > > > > > > > > > > > > it influenced a lot. Finally, the former > > > constructor > > > > > was > > > > > > > > added > > > > > > > > > > back > > > > > > > > > > > > and > > > > > > > > > > > > > > > marked as Deprecated。 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > In conclusion, the goal of this FLIP is to > > annotate > > > > > > > > > > > > > > > SingleThreadFetcherManager(includes its parent > > > class) > > > > > and > > > > > > > > > > > > > > > FutureCompletingBlockingQueue as > PublicEvolving. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Looking forward to hearing from you. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=278465498 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [2] > > > > https://issues.apache.org/jira/browse/FLINK-31324 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [3] > > > > https://issues.apache.org/jira/browse/FLINK-28853 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
