You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Hongshun Wang <lo...@gmail.com> on 2023/12/20 02:07:24 UTC
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
Posted by Hongshun Wang <lo...@gmail.com>.
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 <be...@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 <lo...@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 <re...@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 <be...@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 <loserwang1024@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
>>>> >
>>>>
>>>
Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving
Posted by Becket Qin <be...@gmail.com>.
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 <lo...@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 <re...@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 <be...@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 <lo...@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
>>> >
>>>
>>
Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving
Posted by Hongshun Wang <lo...@gmail.com>.
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 <re...@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 <be...@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 <lo...@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
>> >
>>
>
Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving
Posted by Qingsheng Ren <re...@gmail.com>.
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 <be...@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 <lo...@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
> >
>
Re: [DISCUSS] FLIP-389: Annotate SingleThreadFetcherManager and FutureCompletingBlockingQueue as PublicEvolving
Posted by Becket Qin <be...@gmail.com>.
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 <lo...@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
>