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/#/ > > > > > >>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > > > > > >
