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) Qin On Wed, Jan 10, 2024 at 10:39 PM Hongshun Wang <loserwang1...@gmail.com> wrote: > 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 consider > doing it later, rather than in this FLIP. > > > Adding a new internal method, SplitFetcherManager#getQueue(), to > SourceReaderBase seems to be a better option than exposing methods like > poll and notifyAvailable on SplitFetcherManager. > > > I have taken this valuable suggestion and updated the FLIP accordingly. > > > Thanks, > > Hongshun > > On Thu, Jan 11, 2024 at 2:09 PM Qingsheng Ren <renqs...@gmail.com> wrote: > >> 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 developers so that >> they can focus on the interaction with external systems. >> >> However in the current FLIP, SplitFetcherManager exposes 4 more methods, >> poll / getAvailabilityFuture / notifyAvailable / noAvailableElement, which >> are tightly coupled with the implementation of the elementQueue. The naming >> of these methods look weird to me, like what does it mean to "poll from a >> SplitFetcherManager" / "notify a SplitFetcherManager available"? To clarify >> these methods we have to explain to developers that "well we hide a queue >> inside SplitFetcherMamager and the poll method is actually polling from the >> queue". I'm afraid these methods will implicitly expose the concept and the >> implementation of the queue to developers. >> >> I think a cleaner solution would be having a new interface that extracts >> SplitFetcher creating and managing logic from the current >> SplitFetcherManager, but having too many concepts might make the entire >> Source API even harder to understand. To make a compromise, I'm considering >> only exposing constructors of SplitFetcherManager as public APIs, and >> adding a new internal method SplitFetcherManager#getQueue() for >> SourceReaderBase (well it's a bit hacky I admit but I think exposing >> methods like poll and notifyAvailable on SplitFetcherManager is even >> worth). WDTY? >> >> Thanks, >> Qingsheng >> >> On Thu, Dec 21, 2023 at 8:36 AM Becket Qin <becket....@gmail.com> wrote: >> >>> 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 the public >>> interface section instead. >>> >>> 2. It would be good to put all the changes of one class together. For >>> example, for SplitFetcherManager, we can say: >>> - Change SplitFetcherManager from Internal to PublicEvolving. >>> - Deprecate the old constructor exposing the >>> FutureCompletingBlockingQueue, and add new constructors as replacements >>> which creates the FutureCompletingBlockingQueue instance internally. >>> - Add a few new methods to expose the functionality of the internal >>> FutureCompletingBlockingQueue via the SplitFetcherManager. >>> And then follows the code block containing all the changes above. >>> Ideally, the changes should come with something like "// <------ New", so >>> that it is. easier to be found. >>> >>> 3. In the proposed changes section, it would be good to add some more >>> detailed explanation of the idea behind the public interface changes. So >>> even people new to Flink can understand better how exactly the interface >>> changes will help fulfill the motivation. For example, regarding the >>> constructor signature change, we can say the following. We can mention a >>> few things in this section: >>> - By exposing the SplitFetcherManager / SingleThreadFetcheManager, by >>> implementing addSplits() and removeSplits(), connector developers can >>> easily create their own threading models in the SourceReaderBase. >>> - Note that the SplitFetcher constructor is package private, so users >>> can only create SplitFetchers via >>> SplitFetcherManager.createSplitFetcher(). >>> This ensures each SplitFetcher is always owned by the >>> SplitFetcherManager. >>> - This FLIP essentially embedded the element queue (a >>> FutureCompletingBlockingQueue) instance into the SplitFetcherManager. >>> This >>> hides the element queue from the connector developers and simplifies the >>> SourceReaderBase to consist of only SplitFetcherManager and RecordEmitter >>> as major components. >>> >>> In short, the public interface section answers the question of "what". We >>> should list all the user-sensible changes in the public interface >>> section, >>> without verbose explanation. The proposed changes section answers "how", >>> where we can add more details to explain the changes listed in the public >>> interface section. >>> >>> Thanks, >>> >>> Jiangjie (Becket) Qin >>> >>> >>> >>> On Wed, Dec 20, 2023 at 10:07 AM Hongshun Wang <loserwang1...@gmail.com> >>> wrote: >>> >>> > 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 >>> > >>> >>
