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