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, Jan 12, 2024 at 1:00 AM Becket Qin <becket....@gmail.com> wrote:
> 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
>>>> >
>>>>
>>>
