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
>
