Hello guys and thank you for your feedback! Very well appreciated. Sorry for the late reply, but it took a lot of time to address your concerns. I had to re-design the refactor and convince myself of the best way to accomodate both rewriting existing connectors (e.g. Postgres) and new ones. Hope you like it.
You can have a look at the previous document [1] to inspect the changes. Here are the detailed answers to your questions: @Keith Lee > 1. Can the FLIP include at least one implementation of JDBC based connector > using proposed changes? Implementing a connector and solving challenges that > arise using the proposed change will give good insight. You can now find an implementation of both ExampleDB and Postgres Sink. > 2. 2. The example seems to restrict the connector to managed database service > provider at `exampledb.com`. I think generally database can be self hosted > e.g. localhost or managed by a cloud service provider. If we use the example > configuration pattern, we are actually limiting the connector usage to > managed exampledb, IMO we should not really do that. Can you provide other > example configuration (actual examples for MySQL/Postgres etc. would be > great) that may illustrate the usefulness of the proposal better? I don't see how that limits the connector to managed services. In any case, you can now find an example usage of Postgres Sink via TableAPI. @Leonard Xu > (1) Please add API annotation for all Proposed public interfaces Addressed. > (2) > JdbcConnectionOptionsParser/JdbcReadOptionsParser/JdbcExecutionOptionsParser > offer two methods validate and parse, it’s a little stranger to me as your > POC code call them at the same time, could we finish validate action in parse > method internal? And thus a Parser interface offers a parse method makes > sense to me. It’s better introduce a Validator to support validation If you > want to do some connection validations during job compile phase. I now articulate in the FLIP why validation and parse methods are tightly bound together. This is for segregating everything that revolves around some option category (e.g.: connection or execution) into one interface. This has the benefit to increase code readability and maintenance of the already existing options categories. In general, we don't want a user to be able to compose different parsing and validation steps, as this could cause troubles! Imagine that one applies validation logic A with parsing logic B: the latter could give for granted that validation A went through while it did not! So, it makes sense to tightly bound them together. Instead of leaving the `parse` implementation perform the validation as well, I preferred to separate the two as it would also make it explicit to the user to perform validation. Also, not all interfaces have a single `parse` method and that would make the user uncertain on were to apply the validation logic. > (3) Above methods return InternalJdbcConnectionOptions with fixed members, if the example db requires extra connection options like acessKey, acessId and etc, we need to change InternalJdbcConnectionOptions as well, how we show our extensibility？ I fixed now the `ConnectionOptionParser` to not use the `InternalJdbcConnectionOptionsBuilder` as we should not make users use internal classes as well. Now the required connection parameters are requested one by one. For extra options, the `getProperties` method is introduced. Hope this clarifies everything :) Looking forward for your next review. Thanks, Lorenzo On May 21, 2024 at 16:43 +0200, Keith Lee <leekeiabstract...@gmail.com>, wrote: > Hi Lorenzo > > I have a couple of questions: > > 1. Can the FLIP include at least one implementation of JDBC based connector > using proposed changes? Implementing a connector and solving challenges > that arise using the proposed change will give good insight. > 2. The example seems to restrict the connector to managed database service > provider at `exampledb.com`. I think generally database can be self hosted > e.g. localhost or managed by a cloud service provider. If we use the > example configuration pattern, we are actually limiting the connector usage > to managed exampledb, IMO we should not really do that. Can you provide > other example configuration (actual examples for MySQL/Postgres etc. would > be great) that may illustrate the usefulness of the proposal better? > > > Best regards > Keith Lee > > > On Tue, May 21, 2024 at 2:01 PM Leonard Xu <xbjt...@gmail.com> wrote: > > > Thanks Lorenzo for kicking off this discussion. > > > > +1 for the motivation, and I left some comments as following: > > > > (1) Please add API annotation for all Proposed public interfaces > > > > (2) > > JdbcConnectionOptionsParser/JdbcReadOptionsParser/JdbcExecutionOptionsParser > > offer two methods validate and parse, it’s a little stranger to me as your > > POC code call them at the same time, could we finish validate action in > > parse method internal? And thus a Parser interface offers a parse method > > makes sense to me. It’s better introduce a Validator to support validation > > If you want to do some connection validations during job compile phase. > > > > (3) Above methods return InternalJdbcConnectionOptions with fixed members, > > if the example db requires extra connection options like acessKey, acessId > > and etc, we need to change InternalJdbcConnectionOptions as well, how we > > show our extensibility？ > > > > Best, > > Leonard > > > > > > > > 2024年5月15日 下午10:17，Ahmed Hamdy <hamdy10...@gmail.com> 写道： > > > > > > > > Hi Lorenzo, > > > > This seems like a very useful addition. > > > > +1 (non-binding) from my side. I echo Jeyhun's question about backward > > > > compatibility as it is not mentioned in the FLIP. > > > > Best Regards > > > > Ahmed Hamdy > > > > > > > > > > > > On Wed, 15 May 2024 at 08:12, <lorenzo.affe...@ververica.com.invalid> > > wrote: > > > > > > > > >> Hello Muhammet and Jeyhun! > > > > >> Thanks for your comments! > > > > >> > > > > >> @Jeyhun: > > > > >> > > > > > >>> Could you please elaborate more on how the new approach will be > > backwards > > > > >> compatible? > > > > >> > > > > >> In the FLIP I provide how the current Factories in JDBC would be > > > > >> changed > > > > >> with this refactor, do you mean something different? Can you be more > > > > >> specific with your request? > > > > >> On May 14, 2024 at 12:32 +0200, Jeyhun Karimov > > > > >> <je.kari...@gmail.com>, > > > > >> wrote: > > > > > >>> Hi Lorenzo, > > > > > >>> > > > > > >>> Thanks for driving this FLIP. +1 for it. > > > > > >>> > > > > > >>> Could you please elaborate more on how the new approach will be > > backwards > > > > > >>> compatible? > > > > > >>> > > > > > >>> Regards, > > > > > >>> Jeyhun > > > > > >>> > > > > > >>> On Tue, May 14, 2024 at 10:00 AM Muhammet Orazov > > > > > >>> <mor+fl...@morazow.com.invalid> wrote: > > > > > >>> > > > > > > >>>> Hey Lorenzo, > > > > > > >>>> > > > > > > >>>> Thanks for driving this FLIP! +1 > > > > > > >>>> > > > > > > >>>> It will improve the user experience of using JDBC based > > > > > > >>>> connectors and help developers to build with different drivers. > > > > > > >>>> > > > > > > >>>> Best, > > > > > > >>>> Muhammet > > > > > > >>>> > > > > > > >>>> On 2024-05-13 10:20, lorenzo.affe...@ververica.com.INVALID > > > > > > >>>> wrote: > > > > > > > > >>>>>> Hello dev! > > > > > > > > >>>>>> > > > > > > > > >>>>>> I want to share a draft of my FLIP to refactor the JDBC > > > > > > > > >>>>>> connector > > > > >> to > > > > > > > > >>>>>> improve its extensibility [1]. > > > > > > > > >>>>>> The goal is to allow implementers to write new > > > > > > > > >>>>>> connectors on top > > > > >> of the > > > > > > > > >>>>>> JDBC one for Table API with clean and maintainable code. > > > > > > > > >>>>>> > > > > > > > > >>>>>> Any feedback from the community is more and welcome. > > > > > > > > >>>>>> > > > > > > > > >>>>>> [1] > > > > > > > > >>>>>> > > > > > > >>>> > > > > >> > > https://docs.google.com/document/d/1kl_AikMlqPUI-LNiPBraAFVZDRg1LF4bn6uiNtR4dlY/edit?usp=sharing > > > > > > >>>> > > > > >> > > > >
