Hi all, This feels like we purposely want to abandon the idea that was introduced with FLIP-33 on standardizing metrics [1]. From this proposal, I don't see why we want to abandon that idea. Next to that, if you override the numRecordsIn logic, you also touch on the other metrics that rely on this value.
Best regards, Martijn [1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-33%3A+Standardize+Connector+Metrics On Sun, Jan 8, 2023 at 12:43 PM Wencong Liu <liuwencle...@163.com> wrote: > Thanks for the discussion, Dong and John > > > Considering the extra cost of method calls, the approach of adding an > extra SourceReaderBase constructor > should be a better choice. If there is no further discussion, I will > follow this plan. > > > Best, > Wencong > > > > > > > > > > > > > > > > > > At 2023-01-08 01:05:07, "John Roesler" <vvcep...@apache.org> wrote: > >Thanks, for the discussion, Dong. > > > >To answer your question: I was unclear if the desire was to deprecate the > metric itself, to be replaced with some other metric, or whether we just > wanted to deprecate the way the superclass manages it. It’s clear now that > we want to keep the metric and only change the superclass logic. > > > >I think we’re on the same page now. > > > >Thanks! > >-John > > > >On Sat, Jan 7, 2023, at 07:21, Dong Lin wrote: > >> Hi John, > >> > >> Not sure if I understand the difference between "deprecate that metric" > and > >> "deprecate the private counter mechanism". I think what we want is to > >> update SourceReaderBase's implementation so that it no longer explicitly > >> increments this metric. But we still need to expose this metric to user. > >> And methods such as RecordEmitter#emitRecord (which can be invoked by > >> SourceReaderBase#pollNext(...)) can potentially increment this metric. > >> > >> I like your approach of adding an extra SourceReaderBase constructor. > That > >> appears simpler than adding a deprecated config. We can mark the > existing > >> SourceReaderBase constructor as @deprecated. > SourceReaderBase#pollNext(...) > >> will not increment the counter if a subclass uses the newly added > >> constructor. > >> > >> Cheers, > >> Dong > >> > >> > >> On Sat, Jan 7, 2023 at 4:47 AM John Roesler <vvcep...@apache.org> > wrote: > >> > >>> Thanks for the replies, Dong and Wencong! > >>> > >>> That’s a good point about the overhead of the extra method. > >>> > >>> Is the desire to actually deprecate that metric in a user-facing way, > or > >>> just to deprecate the private counter mechanism? > >>> > >>> It seems like if the desire is to deprecate the existing private > counter, > >>> we can accomplish it by deprecating the current constructor and > offering > >>> another that is documented not to track the metric. This seems better > than > >>> the config option, since the concern is purely about the division of > >>> responsibilities between the sub- and super-class. > >>> > >>> Another option, which might be better if we wish to keep a uniformly > named > >>> metric, would be to simply make the counter protected. That would be > better > >>> if we’re generally happy with the metric and counter, but a few special > >>> source connectors need to emit records in other ways. > >>> > >>> And finally, if we really want to get rid of the metric itself, then I > >>> agree, a config is the way to do it. > >>> > >>> Thanks, > >>> John > >>> > >>> On Fri, Jan 6, 2023, at 00:55, Dong Lin wrote: > >>> > Hi John and Wencong, > >>> > > >>> > Thanks for the reply! > >>> > > >>> > It is nice that optional-2 can address the problem without affecting > the > >>> > existing source connectors as far as functionality is concerned. One > >>> > potential concern with this approach is that it might increase the > Flink > >>> > runtime overhead by adding one more virtual functional call to the > >>> > per-record runtime call stack. > >>> > > >>> > Since Java's default MaxInlineLevel is 12-18, I believe it is easy > for an > >>> > operator chain of 5+ operators to exceed this limit. In this case. > And > >>> > option-2 would incur one more virtual table lookup to produce each > >>> record. > >>> > It is not clear how much this overhead would show up for jobs with a > >>> chain > >>> > of lightweight operators. I am recently working on FLINK-30531 > >>> > <https://issues.apache.org/jira/browse/FLINK-30531> to reduce > runtime > >>> > overhead which might be related to this discussion. > >>> > > >>> > In comparison to option-2, the option-3 provided in my earlier email > >>> would > >>> > not add this extra overhead. I think it might be worthwhile to > invest in > >>> > the long-term performance (and simpler runtime infra) and pay for the > >>> > short-term cost of deprecating this metric in SourceOperatorBase. > What do > >>> > you think? > >>> > > >>> > Regards, > >>> > Dong > >>> > > >>> > > >>> > On Thu, Jan 5, 2023 at 10:10 PM Wencong Liu <liuwencle...@163.com> > >>> wrote: > >>> > > >>> >> Hi, All > >>> >> > >>> >> > >>> >> Thanks for the reply! > >>> >> > >>> >> > >>> >> I think both John and Dong's opinions are reasonable. John's > Suggestion > >>> 2 > >>> >> is a good implementation. > >>> >> It does not affect the existing source connectors, but also provides > >>> >> support > >>> >> for custom counter in the future implementation. > >>> >> > >>> >> > >>> >> WDYT? > >>> >> > >>> >> > >>> >> Best, > >>> >> > >>> >> Wencong Liu > >>> >
