Hi all, Generally I'm fine with the design. Before I cast my +1 I wanted to clarify one thing. Is the module name in ModuleFactory#createModule necessary? Can't it be just?:
interface ModuleFactory extends TableFactory {
Module createModule(Map<String, String> properties);
}
The name under which the module was registered should not affect the
implementation of the module as far as I can tell. Could we remove this
parameter from the method?
I also spotted a few "bugs" in the design, but they do not affect the
outcome of the design, as they are either just artifacts of refactoring
the FLIP or affect only the internal implementation:
* there is a typo in the ModuleFactory#createModule return type. It
should be Module instead of Plugin
* the return type of ModuleManager:listFunctions() should be
Set<String> instead of Set<Set<String>>, right?
* we cannot use list to store the modules in ModuleManager if I am not
mistaken. We need to store them in a Map to e.g. be able to unload
the modules by its name.
Best,
Dawid
On 17/10/2019 04:16, Jark Wu wrote:
> +1
>
> Thanks,
> Jark
>
> On Thu, 17 Oct 2019 at 04:44, Peter Huang <huangzhenqiu0...@gmail.com>
> wrote:
>
>> +1 Thanks
>>
>> On Wed, Oct 16, 2019 at 12:48 PM Xuefu Z <usxu...@gmail.com> wrote:
>>
>>> +1 (non-biding)
>>>
>>> On Wed, Oct 16, 2019 at 2:26 AM Timo Walther <twal...@apache.org> wrote:
>>>
>>>> +1
>>>>
>>>> Thanks,
>>>> Timo
>>>>
>>>>
>>>> On 15.10.19 20:50, Bowen Li wrote:
>>>>> Hi all,
>>>>>
>>>>> I'd like to kick off a voting thread for FLIP-68: Extend Core Table
>>>> System
>>>>> with Pluggable Modules [1], as we have reached consensus in [2].
>>>>>
>>>>> The voting period will be open for at least 72 hours, ending at 7pm
>> Oct
>>>> 18
>>>>> UTC.
>>>>>
>>>>> Thanks,
>>>>> Bowen
>>>>>
>>>>> [1]
>>>>>
>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-68%3A+Extend+Core+Table+System+with+Pluggable+Modules
>>>>> [2] https://www.mail-archive.com/dev@flink.apache.org/msg29894.html
>>>>>
>>>>
>>> --
>>> Xuefu Zhang
>>>
>>> "In Honey We Trust!"
>>>
signature.asc
Description: OpenPGP digital signature
