Hi Qingsheng, Sorry for my wrong format. > If I understand correctly these are specified in DDL table options by users.
It's inconvenient for user to checkout the options when they in front of a running task. And they don't know the real underlying options in effect if there are some bugs or other incorrect configurations lead to invalid. > I don’t think there's a rule that all metric names should be in MetricNames > class, but it would be great to aggregate these constants into a unified > place. It's a good choice to aggregate the constants together. Best regards, Yuan At 2022-03-08 09:57:30, "Qingsheng Ren" <renqs...@gmail.com> wrote: >Hi Yuan, > >> how can we tell the real “identifier” and “type” options in effect to users? > >If I understand correctly these are specified in DDL table options by users. >For example: > >CREATE TABLE DimTable (…) WITH ( > ... > “cache.identifier” = “guava”, > “cache.type” = “LRU” >); > >> Does MetricNames.java contain all metric names? > > >I don’t think there's a rule that all metric names should be in MetricNames >class, but it would be great to aggregate these constants into a unified >place. > >Cheers, > >Qingsheng > > >> On Mar 8, 2022, at 10:22, zst...@163.com wrote: >> >> 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
