Hi Qingsheng,
> 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
