Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Devs, Thanks for all your advice, it helps a lot. I have already revised the document[1] and started a vote[2]. Thanks, Hongshun [1] https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=278465498 [2] https://lists.apache.org/thread/t1zff21z440pvv48jyhm8pgtqsyplchn On Fri,

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Qingsheng, Thanks for the comment. I think the initial idea is to hide the queue completely from the users, i.e. make FutureCompletingBlockingQueue class internal. If it is OK to expose the class to the users, then just returning the queue sounds reasonable to me. Thanks, Jiangjie (Becket) Qi

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Qingsheng, I agree with you that it would be clearer to have a new interface that extracts the SplitFetcher creation and management logic from the current SplitFetcherManager. However, extensive modifications to the interface may influence a lot and cause compatibility issues. Perhaps we can c

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Hongshun and Becket, Sorry for being late in the discussion! I went through the entire FLIP but I still have some concerns about the new SplitFetcherManager. First of all I agree that we should hide the elementQueue from connector developers. This could simplify the interface exposed to develo

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Hongshun, I think the proposal in the FLIP is basically fine. A few minor comments: 1. In FLIPs, we define all the user-sensible changes as public interfaces. The public interface section should list all of them. So, the code blocks currently in the proposed changes section should be put into

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Becket, It has been a long time since we last discussed. Are there any other problems with this Flip from your side? I am looking forward to hearing from you. Thanks, Hongshun Wang

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 wrote: > Hi Becket, > > Thanks a lot,

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 FutureCompl

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 cla

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 clas

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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.

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 c

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 wi

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi Hongshun, > > > However, it will be tricky because SplitFetcherManager includes extends SourceSplit>, while FutureCompletingBlockingQueue includes . > This means that SplitFetcherManager would have to be modified to SplitT extends SourceSplit>, which would affect the compatibility of the > Spl

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 FutureComple

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 o

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

@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 miss

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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 Interna

Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

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

[DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving

Hi devs, I would like to start a discussion on FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving.[ 1]. Though the SingleThreadFetcherManager is annotated as Internal, it ac