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