Hi Wencong, Thanks for the proposal! I agree that we should fix this disconnect between the metric and what is actually happening in those source connectors.
My instincts agree with Dong's. Adding a configuration option in order to tune the relationship between a superclass and a subclass seems not quite right. The best way to customize superclass behavior is to override a method, and the second best approach is to add a constructor with a new parameter. Override Suggestion 1: It looks like if the source connector overrides `pollNext`, you can prevent the undesired `numRecordsInCounter.inc(1)` call. If you simply want to disable the way the base class emits (and counts) records and do it another way, this might be your best bet. If you still want to invoke the counter on your own, you could propose to make it `protected` as well. Override Suggestion 2: If you want to ensure that the metric is always incremented when a record is emitted, you can propose to add a new protected method like `SourceReaderBase#emitRecord`, which increments the counter and then calls the recordEmitter. `pollNext` would be updated to call this method, and any other subclass logic could also call the method. Constructor Suggestion: This option isn't as good, but you could add a new constructor that takes a boolean flag to disable the default counting logic, similar to the config you propose. The advantage is that no one besides the base class and subclasses have to be aware of this flag, and the behavior is configured at compile time instead of runtime. The disadvantage is that the base class will need new conditional checks and branches, which increases its complexity for all source connectors, not just the ones that want to change the default behavior. Looking at all these options, I would personally prefer Override Suggestion 2. I hope this helps! Thanks, -John On 2023/01/04 12:58:13 Dong Lin wrote: > Hi Wencong, > > Thanks for kicking off the discussion! I think it would be nice to address > this problem. > > Is the config supposed to be publicly visible only to source connector > developers but not to end users? It might be a bit unusual to have a > subclass use a config to disable a public feature in the superclass. > > One alternative solution might be to deprecate/remove numRecordsInCounter > from SourceReaderBase entirely and ask developers to maintain the counter > as appropriate. For KafkaSource and FileSource, the counter can be > incremented in KafkaSourceRecordEmitter#emitRecord and > FileSourceRecordEmitter#emitRecord respectively. We would still need to add > a config in the short term to allow a deprecation period. And the config > itself will be marked as @deprecated. > > The downside of this alternative solution is that we need to deprecate the > counting feature in SourceReaderBase. The upside is that we won't need to > add a public config and the implementation might be easier to maintain in > the long term. > > What do you think? > > Thanks, > Dong > > > On Tue, Jan 3, 2023 at 11:50 AM Wencong Liu <liuwencle...@163.com> wrote: > > > Hi devs, > > > > > > I'd like to start a discussion about FLINK-30234: SourceReaderBase should > > provide an option to disable numRecordsIn metric registration [1]. > > > > > > As the FLINK-302345 describes, the numRecordsIn metric is pre-registered > > for all sources in SourceReaderBase currently. Considering different > > implementation of source reader, the definition of "record" might differ > > from the one we use in SourceReaderBase, hence numRecordsIn might be > > inaccurate. > > > > > > We could introduce an public option in SourceReaderOptions used in > > SourceReaderBase: > > > > > > source.reader.metric.num_records_in.override: false > > > > > > By default, the source reader will use the numRecordsIn metric in > > SourceReaderBase. If source reader want to report to metric by self, it can > > set source.reader.metric.num_records_in.override to true, which disables > > the registration of numRecordsIn in SourceReaderBase and let the actual > > implementation to report the metric instead. > > > > > > Any thoughts on this? > > > > > > [1] > > https://issues.apache.org/jira/browse/FLINK-30234?jql=project%20%3D%20FLINK > > > > > > Best, Wencong Liu >
