Let me chime-in and add comments regarding the public interface section. Please see my comments inline.
On Thu, Dec 29, 2022 at 6:08 PM Jark Wu <imj...@gmail.com> wrote: > 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. > SplitEnumeratorContext#metricGroup is an interface and this FLIP does not seem to change its semantics/behavior. The FLIP does change the implementation/behavior of SourceCoordinatorContext#metricGroup, which is marked @Internal. Thus it might seem a bit weird to add in the public interface section saying that we change the interface SplitEnumeratorContext#metricGroup from returning null to non-null object. 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. Since OperatorCoordinator is an internal API, it seems reasonable to explain it in the proposed change section. The FLIP seems to have documented this in point 5 of the proposed change section. BTW, if we think there are @internal classes that are important enough to be added in the public interface section, it might be useful to explicitly discuss this topic and document it in the "*What are the "public interfaces" of the project*" in this <https://cwiki.apache.org/confluence/display/FLINK/Flink+Improvement+Proposals> wiki. > 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 > > > > >>> > > >> > > > > >>> > > > > > > >>> > > > > > > > > > > > >
