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