Hi all, Any additional questions or concern regarding this FLIP-389[1].? 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 3:44 PM Hongshun Wang <loserwang1...@gmail.com> wrote: > 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 >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> >
