Hi Qingsheng Ren, Thanks for your feedback.
> 1. It looks like “identifier” and “type” are options of cache instead of > metrics. I think they are finalized once the cache is created so maybe it’s > not quite helpful to report them to the metric system. Maybe it's not quite appropriate to report them to the metric system, but how can we tell the real “identifier” and “type” options in effect to users? > 2. Some metrics can be aggregated simply in metric systems, like loadCount = > loadSuccessCount + loadExceptionCount, so maybe we can just keep fundamental > metrics (like loadSuccessCount and loadExceptionCount) to avoid redundancy. I agree with you. I have removed these redundant metrics. > 3. About the interface of CacheMetricGroup I think it would be easier for > cache implementers to use if we expose wrapped function instead of let users > provide gauges directly. Thanks for your advice, they are helpful and I have adjusted it. I have a question about it. Does MetricNames.java <https://github.com/apache/flink/blob/master/flink-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fflink%2Fruntime%2Fmetrics%2FMetricNames.java> contain all metric names? Should I put the cache metric names here? Best regards, Yuan At 2022-03-07 16:55:18, "Qingsheng Ren" <renqs...@gmail.com> wrote: >Hi Yuan, > >Thanks for raising this discussion! I believe this will be very helpful for >lookup table developers, and standardizing metrics would be essential for >users to tuning their systems. > >Here’s some thoughts in my mind: > >1. It looks like “identifier” and “type” are options of cache instead of >metrics. I think they are finalized once the cache is created so maybe it’s >not quite helpful to report them to the metric system. > >2. Some metrics can be aggregated simply in metric systems, like loadCount = >loadSuccessCount + loadExceptionCount, so maybe we can just keep fundamental >metrics (like loadSuccessCount and loadExceptionCount) to avoid redundancy. > >3. About the interface of CacheMetricGroup I think it would be easier for >cache implementers to use if we expose wrapped function instead of let users >provide gauges directly. For example: > >public interface CacheMetricGroup extends MetricGroup { > // Mark a cache hit > public void markCacheHit(); > // Mark a cache miss > public void recordCacheMiss(); > ... >} > >You can check SourceReaderMetricGroup[1] and its implementation[2] as a >reference. > >Hope these would be helpful! > >Best regards, > >Qingsheng Ren > >[1] >https://github.com/apache/flink/blob/master/flink-metrics/flink-metrics-core/src/main/java/org/apache/flink/metrics/groups/SourceReaderMetricGroup.java >[2] >https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/InternalSourceReaderMetricGroup.java > > >> On Mar 7, 2022, at 16:00, zst...@163.com wrote: >> >> Hi devs, >> >> >> I would like to propose a discussion thread about abstraction of Cache >> LookupFunction with metrics for cache in connectors to make cache out of box >> for connector developers. There are multiple LookupFunction implementations >> in individual connectors [1][2][3][4] so far. >> At the same time, users can monitor cache in LookupFunction by adding >> uniform cache metrics to optimize tasks or troubleshoot. >> >> >> I have posted an issue about this, see >> <https://issues.apache.org/jira/browse/FLINK-25409>, and made a brief design >> <https://docs.google.com/document/d/1L2eo7VABZBdRxoRP_wPvVwuvTZOV9qrN9gEQxjhSJOc/edit?usp=sharing>. >> >> >> Looking forward to your feedback, thanks. >> >> >> Best regards, >> Yuan >> >> >> >> >> [1] >> https://github.com/apache/flink/blob/master/flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcRowDataLookupFunction.java >> [2] >> https://github.com/apache/flink/blob/master/flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/connectors/hive/FileSystemLookupFunction.java >> [3] >> https://github.com/apache/flink/blob/master/flink-connectors/flink-connector-hbase-base/src/main/java/org/apache/flink/connector/hbase/source/HBaseRowDataLookupFunction.java >> [4] >> https://github.com/apache/flink/blob/master/flink-connectors/flink-connector-hbase-2.2/src/main/java/org/apache/flink/connector/hbase2/source/HBaseRowDataAsyncLookupFunction.java
