Hi Hang, Thanks for driving this discussion. I think this is a very useful feature for connectors.
The FLIP looks quite good to me, and I just have two suggestions. 1. In the "Public Interface" section, mention that the implementation behavior of "SplitEnumeratorContext#metricGroup" is changed from returning null to returning a concrete SplitEnumeratorMetricGroup instance. Even though the API is already there, the behavior change can also be considered a public change. 2. Mention the newly added interface of "OperatorCoordinator#metricGroup" in the "Proposed Changes" section or "Public Interface" section. As the FLIP said, OperatorCoordinator is widely used in many connectors. Though it is still an @Internal API, I think it is worth mentioning the change in the FLIP. Best, Jark On Mon, 26 Dec 2022 at 18:06, Hang Ruan <ruanhang1...@gmail.com> wrote: > Hi, thanks for the feedback, Zhu Zhu and Qingsheng. > After combining everyone's comments, the main concerns and corresponding > adjustments are as follows. > > Q1: Common metrics are not quite useful. > numEventsIn and numEventsOut counters will be removed from the > OperatorCoordinatorMetricGroup. These common metrics do not provide enough > information for users. The users are more willing to get the number of > events of the specified type instead of the total number. And this metric > is calculated differently. The implementation could register the metric by > themselves. > > Q2: This FLIP is overly complicated. > This FLIP will become concise after these modifications. > OperatorCoordinatorMetricGroup has already been introduced into Flink by > FLIP-179< > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-179%3A+Expose+Standardized+Operator+Metrics > >. > And > this FLIP will not change it. This FLIP only provides a new metric option > and a new metric group scope. The changes in proposed changes provide the > details about the modifications for the internal classes, which might make > it look complicated. > > Thanks for all the comments again. If there are no further comments, we > plan to start the voting thread this week. > > Best, > Hang > > Qingsheng Ren <renqs...@gmail.com> 于2022年12月26日周一 16:48写道： > > > Thanks for the FLIP, Hang! > > > > This FLIP overall looks good to me. Actually I share the same concern > with > > Zhu that numEventsIn and numEventsOut counters are not quite useful to > end > > users. OperatorEvent is a quite low-level abstraction, which requires > > instantialization in order to be practical to users and developers, so > > maybe it's better to exclude them from the FLIP. > > > > Best, > > Qingsheng > > Ververica (Alibaba) > > > > On Mon, Dec 26, 2022 at 12:08 PM Zhu Zhu <reed...@gmail.com> wrote: > > > > > Hi Hang, > > > I still see no strong reason why we need numEventsIn/numEventsOut > > metrics. > > > In the discussion in FLINK-29801, I can see the same concern from > others. > > > So I prefer to exclude them from this FLIP to avoid over-extending the > > > scope. > > > > > > Thanks, > > > Zhu > > > > > > Hang Ruan <ruanhang1...@gmail.com> 于2022年12月23日周五 15:21写道： > > > > > > > > Hi everyone, > > > > > > > > As for the Zhu Zhu's problem, I think we should keep the common > > metrics, > > > which will help to observe incoming and outgoing events. What do you > > think, > > > @Zhu Zhu ? > > > > And @Chesnay, are there any other issues you are more concerned > about? > > > Looking forward to your reply. > > > > > > > > Thanks for all the comments. If there are no further comments, we > plan > > > to start the voting thread next week. > > > > > > > > Best, > > > > Hang > > > > > > > > Hang Ruan <ruanhang1...@gmail.com> 于2022年12月15日周四 16:49写道： > > > >> > > > >> Hi, Zhu Zhu, > > > >> > > > >> Thanks for your feedback! > > > >> > > > >> The OperatorCoordinator implementations are different. And their > > > metrics are much different too. We try to find the common metrics and > put > > > them in the OperatorCoordinatorMetricGroup. If most developers think we > > do > > > not need these common metrics, removing the common metrics is also > > > acceptable. > > > >> > > > >> Best, > > > >> Hang > > > >> > > > >> Zhu Zhu <reed...@gmail.com> 于2022年12月14日周三 22:09写道： > > > >>> > > > >>> Hi Hang & MengYue, > > > >>> > > > >>> Thanks for creating this FLIP! > > > >>> > > > >>> I think it is very useful, mainly in two aspects: > > > >>> 1. Enables OperatorCoordinators to register metrics. Currently > > > >>> the coordinators has no way to do this. And operator coordinator > > > >>> metric group further enables the SplitEnumerator to have access > > > >>> to a registered metric group (via the existing public interface > > > >>> SplitEnumeratorContext#metricGroup()), which is null at the moment. > > > >>> > > > >>> 2. Defines the scope of operator coordinator metrics. A clear > > > definition > > > >>> makes it easy for users to find their wanted metrics. The > definition > > > >>> also helps to avoid conflicts of metrics from multiple > > > OperatorCoordinators > > > >>> of the same kind. E.g. each SourceCoordinator may have its own > > > >>> numSourceSplits metric, these metrics should not be directly > > registered > > > >>> to the job metric group. > > > >>> > > > >>> What I'm a bit concerned is the necessity of the introduced common > > > metrics > > > >>> numEventsInCounter & numEventsOutCounter. If there any case which > > > strongly > > > >>> requires them? > > > >>> > > > >>> Regarding the concerns of Chesnay, > > > >>> > A dedicated coordinator MG implementation is overkill > > > >>> Directly using the job metric group can result in metric conflicts, > > as > > > mentioned > > > >>> in above #2. > > > >>> > > > >>> Thanks, > > > >>> Zhu > > > >>> > > > >>> Dong Lin <lindon...@gmail.com> 于2022年12月10日周六 14:16写道： > > > >>> > > > >>> > > > > >>> > Hi Chesney, > > > >>> > > > > >>> > Just to double check with you, OperatorCoordinatorMetricGroup > > > (annotated as > > > >>> > @PublicEvolving) has already been introduced into Flink by > FLIP-179 > > > >>> > < > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-179%3A+Expose+Standardized+Operator+Metrics > > > >. > > > >>> > And that FLIP has got you +1.. Do you mean we should remove this > > > >>> > OperatorCoordinatorMetricGroup? > > > >>> > > > > >>> > Regards, > > > >>> > Dong > > > >>> > > > > >>> > On Sat, Dec 10, 2022 at 1:33 AM Chesnay Schepler < > > ches...@apache.org> > > > wrote: > > > >>> > > > > >>> > > As a whole I feel like this FLIP is overly complicated. A > > dedicated > > > >>> > > coordinator MG implementation is overkill; it could just re-use > > the > > > >>> > > existing Task/OperatorMGs to create the same structure we have > on > > > TMs, > > > >>> > > similar to what we did with the Job MG. > > > >>> > > > > > >>> > > However, I'm not convinced that this is required anyway, > because > > > all the > > > >>> > > example metrics you listed can be implemented on the TM side + > > > >>> > > aggregating them in the external metrics backend. > > > >>> > > > > > >>> > > Since I'm on holidays soon, just so no one tries to pull a fast > > > one on > > > >>> > > me, if this were to go to a vote as-is I'd be against it. > > > >>> > > > > > >>> > > > > > >>> > > On 09/12/2022 15:30, Dong Lin wrote: > > > >>> > > > Hi Hang, > > > >>> > > > > > > >>> > > > Thanks for the FLIP! The FLIP looks good and it is pretty > > > informative. > > > >>> > > > > > > >>> > > > I have just two minor comments regarding names: > > > >>> > > > - Would it be useful to rename the config key as > > > >>> > > > *metrics.scope.jm.job.operator-coordinator* for consistency > > with > > > >>> > > > *metrics.scope.jm.job > > > >>> > > > *(which is not named as *jm-job)? > > > >>> > > > - Maybe rename the variable as > > SCOPE_NAMING_OPERATOR_COORDINATOR > > > for > > > >>> > > > simplicity and consistency with SCOPE_NAMING_OPERATOR (which > is > > > not named > > > >>> > > > as SCOPE_NAMING_TM_JOB_OPERATOR)? > > > >>> > > > > > > >>> > > > Cheers, > > > >>> > > > Dong > > > >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > > > On Thu, Dec 8, 2022 at 3:28 PM Hang Ruan < > > ruanhang1...@gmail.com> > > > wrote: > > > >>> > > > > > > >>> > > >> Hi all, > > > >>> > > >> > > > >>> > > >> MengYue and I created FLIP-274[1] Introduce metric group for > > > >>> > > >> OperatorCoordinator. OperatorCoordinator is the coordinator > > for > > > runtime > > > >>> > > >> operators and running on Job Manager. The coordination > > > mechanism is > > > >>> > > >> operator events between OperatorCoordinator and its all > > > operators, the > > > >>> > > >> coordination is more and more using in Flink, for example > many > > > Sources > > > >>> > > and > > > >>> > > >> Sinks depend on the mechanism to assign splits and > coordinate > > > commits to > > > >>> > > >> external systems. The OperatorCoordinator is widely using in > > > flink kafka > > > >>> > > >> connector, flink pulsar connector, flink cdc connector, > flink > > > hudi > > > >>> > > >> connector and so on. > > > >>> > > >> > > > >>> > > >> But there is not a suitable metric group scope for the > > > >>> > > OperatorCoordinator > > > >>> > > >> and not an implementation for the interface > > > >>> > > OperatorCoordinatorMetricGroup. > > > >>> > > >> These metrics in OperatorCoordinator could be how many > > > splits/partitions > > > >>> > > >> have been assigned to source readers, how many files have > been > > > written > > > >>> > > out > > > >>> > > >> by sink writers, these metrics not only help users to know > the > > > job > > > >>> > > progress > > > >>> > > >> but also make big job maintaining easier. Thus we propose > the > > > FLIP-274 > > > >>> > > to > > > >>> > > >> introduce a new metric group scope for OperatorCoordinator > and > > > provide > > > >>> > > an > > > >>> > > >> internal implementation for OperatorCoordinatorMetricGroup. > > > >>> > > >> > > > >>> > > >> Could you help review this FLIP when you get time? Any > > feedback > > > is > > > >>> > > >> appreciated! > > > >>> > > >> > > > >>> > > >> Best, > > > >>> > > >> Hang > > > >>> > > >> > > > >>> > > >> [1] > > > >>> > > >> > > > >>> > > >> > > > >>> > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-274%3A+Introduce+metric+group+for+OperatorCoordinator > > > >>> > > >> > > > >>> > > > > > >>> > > > > > > > >
