Hi all, Thanks for all the valuable feedback. If there's no further question or concern, I will start voting for FLIP-294 [1] later.
[1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-294%3A+Support+Customized+Catalog+Modification+Listener Best, Shammon FY On Fri, Jun 9, 2023 at 9:07 AM Shammon FY <zjur...@gmail.com> wrote: > Thanks Jing, it makes sense to me and I have updated the FLIP > > > Best, > Shammon FY > > > On Thu, Jun 8, 2023 at 11:15 PM Jing Ge <j...@ververica.com.invalid> > wrote: > >> Hi Shammon, >> >> If we take a look at the JDK Event design as a reference, we can even add >> an Object into the event [1]. Back to the CatalogModificationEvent, >> everything related to the event could be defined in the Event. If we want >> to group some information into the Context, we could also consider adding >> the CatalogModificationContext into the Event and make the onEvent() >> method >> cleaner with only one input parameter CatalogModificationEvent, because >> the >> interface CatalogModificationListener is the most often used interface for >> users. Just my two cents. >> >> Best regards, >> Jing >> >> [1] >> >> http://www.java2s.com/example/java-src/pkg/java/util/eventobject-85298.html >> >> On Thu, Jun 8, 2023 at 7:50 AM Shammon FY <zjur...@gmail.com> wrote: >> >> > Hi, >> > >> > To @Jing Ge >> > > Thanks for the clarification. Just out of curiosity, if the context is >> > not part of the event, why should it be the input parameter of each >> onEvent >> > call? >> > >> > I think it's quite strange to put some information in an Event, such as >> a >> > factory identifier for catalog, but they will be used by the listener. >> I >> > place it in the context class and I think it is more suitable than >> directly >> > placing it in the event class. >> > >> > To @Mason >> > > 1. I'm also curious about default implementations. Would >> atlas/datahub be >> > supported by default? >> > >> > We won't do that and external systems such as atlas/datahub need to >> > implement the listener themselves. >> > >> > > 2. The FLIP title is confusing to me, especially in distinguishing it >> > from FLIP-314. Would a better FLIP title be "Support Catalog Metadata >> > Listener" or something alike? >> > >> > Thanks, I think "Support Catalog Modification Listener" will be >> > more suitable, I'll update the title to it. >> > >> > >> > Best, >> > Shammon FY >> > >> > >> > On Thu, Jun 8, 2023 at 12:25 PM Mason Chen <mas.chen6...@gmail.com> >> wrote: >> > >> > > Hi Shammon, >> > > >> > > FLIP generally looks good and I'm excited to see this feature. >> > > >> > > 1. I'm also curious about default implementations. Would >> atlas/datahub be >> > > supported by default? >> > > 2. The FLIP title is confusing to me, especially in distinguishing it >> > from >> > > FLIP-314. Would a better FLIP title be "Support Catalog Metadata >> > Listener" >> > > or something alike? >> > > >> > > Best, >> > > Mason >> > > >> > > On Tue, Jun 6, 2023 at 3:33 AM Jing Ge <j...@ververica.com.invalid> >> > wrote: >> > > >> > > > Hi Shammon, >> > > > >> > > > Thanks for the clarification. Just out of curiosity, if the context >> is >> > > not >> > > > part of the event, why should it be the input parameter of each >> onEvent >> > > > call? >> > > > >> > > > Best regards, >> > > > Jing >> > > > >> > > > On Tue, Jun 6, 2023 at 11:58 AM Leonard Xu <xbjt...@gmail.com> >> wrote: >> > > > >> > > > > Thanks Shammon for the timely update, the updated FLIP looks good >> to >> > > me. >> > > > > >> > > > > Hope to see the vote thread and following FLIP-314 discussion >> thread. >> > > > > >> > > > > Best, >> > > > > Leonard >> > > > > >> > > > > > On Jun 6, 2023, at 5:04 PM, Shammon FY <zjur...@gmail.com> >> wrote: >> > > > > > >> > > > > > Hi, >> > > > > > >> > > > > > Thanks for all the feedback. >> > > > > > >> > > > > > For @Jing Ge, >> > > > > > I forget to update the demo code in the FLIP, the method is >> > > > > > `onEvent(CatalogModificationEvent, CatalogModificationContext)` >> and >> > > > there >> > > > > > is no `onEvent(CatalogModificationEvent)`. I have updated the >> code. >> > > > > Context >> > > > > > contains some additional information that is not part of an >> Event, >> > > but >> > > > > > needs to be used in the listener, so we separate it from the >> event. >> > > > > > >> > > > > > For @Panagiotis, >> > > > > > I think `ioExecutor` make sense to me and I have added it in >> > > > > > `ContextModificationContext`, thanks >> > > > > > >> > > > > > For @Leonard, >> > > > > > Thanks for your input. >> > > > > > 1. I have updated `CatalogModificationContext` as an interface, >> as >> > > well >> > > > > as >> > > > > > Context in CatalogModificationListenerFactory >> > > > > > 2. Configuration sounds good to me, I have updated the method >> name >> > > and >> > > > > > getConfiguration in Context >> > > > > > >> > > > > > For @David, >> > > > > > Yes, you're right. The listener will only be used on the client >> > side >> > > > and >> > > > > > won't introduce a new code path for running per-job/per-session >> > jobs. >> > > > The >> > > > > > listener will be created in `TableEnvironment` and `SqlGateway` >> > which >> > > > > can a >> > > > > > `CatalogManager` with the listener. >> > > > > > >> > > > > > >> > > > > > Best, >> > > > > > Shammon FY >> > > > > > >> > > > > > >> > > > > > On Tue, Jun 6, 2023 at 3:33 PM David Morávek < >> > > david.mora...@gmail.com> >> > > > > > wrote: >> > > > > > >> > > > > >> Hi, >> > > > > >> >> > > > > >> Thanks for the FLIP! Data lineage is an important problem to >> > tackle. >> > > > > >> >> > > > > >> Can you please expand on how this is planned to be wired into >> the >> > > > > >> JobManager? As I understand, the listeners will be configured >> > > globally >> > > > > (per >> > > > > >> cluster), so this won't introduce a new code path for running >> > > per-job >> > > > / >> > > > > >> per-session user code. Is that correct? >> > > > > >> >> > > > > >> Best, >> > > > > >> D >> > > > > >> >> > > > > >> On Tue, Jun 6, 2023 at 9:17 AM Leonard Xu <xbjt...@gmail.com> >> > > wrote: >> > > > > >> >> > > > > >>> Thanks Shammon for driving this FLIP forward, I’ve several >> > comments >> > > > > about >> > > > > >>> the updated FLIP. >> > > > > >>> >> > > > > >>> 1. CatalogModificationContext is introduced as a class >> instead of >> > > an >> > > > > >>> interface, is it a typo? >> > > > > >>> >> > > > > >>> 2. The FLIP defined multiple Map<String, String> config(); >> > > methods >> > > > in >> > > > > >>> some Context classes, Could we use Configuration >> > > > > >> getConfiguration()；Class >> > > > > >>> org.apache.flink.configuration.Configuration is recommend as >> it’s >> > > > > public >> > > > > >>> API and offers more useful methods as well. >> > > > > >>> >> > > > > >>> 3. The Context of CatalogModificationListenerFactory should >> be an >> > > > > >>> interface too, and getUserClassLoder() >> > > > > >>> would be more aligned with flink’s naming style. >> > > > > >>> >> > > > > >>> >> > > > > >>> Best, >> > > > > >>> Leonard >> > > > > >>> >> > > > > >>>> On May 26, 2023, at 4:08 PM, Shammon FY <zjur...@gmail.com> >> > > wrote: >> > > > > >>>> >> > > > > >>>> Hi devs, >> > > > > >>>> >> > > > > >>>> We would like to bring up a discussion about FLIP-294: >> Support >> > > > > >> Customized >> > > > > >>>> Job Meta Data Listener[1]. We have had several discussions >> with >> > > Jark >> > > > > >> Wu, >> > > > > >>>> Leonard Xu, Dong Lin, Qingsheng Ren and Poorvank about the >> > > functions >> > > > > >> and >> > > > > >>>> interfaces, and thanks for their valuable advice. >> > > > > >>>> The overall job and connector information is divided into >> > metadata >> > > > and >> > > > > >>>> lineage, this FLIP focuses on metadata and lineage will be >> > > discussed >> > > > > in >> > > > > >>>> another FLIP in the future. In this FLIP we want to add a >> > > customized >> > > > > >>>> listener in Flink to report catalog modifications to external >> > > > metadata >> > > > > >>>> systems such as datahub[2] or atlas[3]. Users can view the >> > > specific >> > > > > >>>> information of connectors such as source and sink for Flink >> jobs >> > > in >> > > > > >> these >> > > > > >>>> systems, including fields, watermarks, partitions, etc. >> > > > > >>>> >> > > > > >>>> Looking forward to hearing from you, thanks. >> > > > > >>>> >> > > > > >>>> >> > > > > >>>> [1] >> > > > > >>>> >> > > > > >>> >> > > > > >> >> > > > > >> > > > >> > > >> > >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-294%3A+Support+Customized+Job+Meta+Data+Listener >> > > > > >>>> [2] https://datahub.io/ >> > > > > >>>> [3] https://atlas.apache.org/#/ >> > > > > >>> >> > > > > >>> >> > > > > >> >> > > > > >> > > > > >> > > > >> > > >> > >> >
