[ 
https://issues.apache.org/jira/browse/FLINK-4564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15523161#comment-15523161
 ] 

ASF GitHub Bot commented on FLINK-4564:
---------------------------------------

Github user zentol commented on a diff in the pull request:

    https://github.com/apache/flink/pull/2517#discussion_r80475991
  
    --- Diff: 
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/MetricRegistryTest.java
 ---
    @@ -280,4 +282,65 @@ public void testConfigurableDelimiter() {
     
                registry.shutdown();
        }
    +
    +   @Test
    +   public void testConfigurableDelimiterForReporters() {
    +           Configuration config = new Configuration();
    +           config.setString(ConfigConstants.METRICS_REPORTERS_LIST, 
"test1,test2,test3");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test1." + ConfigConstants.METRICS_REPORTER_SCOPE_DELIMITER, "_");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test1." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter.class.getName());
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test2." + ConfigConstants.METRICS_REPORTER_SCOPE_DELIMITER, "-");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test2." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter.class.getName());
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test3." + ConfigConstants.METRICS_REPORTER_SCOPE_DELIMITER, "AA");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test3." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter.class.getName());
    +
    +           MetricRegistry registry = new MetricRegistry(config);
    +
    +           assertEquals('.', registry.getDelimiter());
    +           assertEquals('_', registry.getDelimiter(0));
    +           assertEquals('-', registry.getDelimiter(1));
    +           assertEquals('.', registry.getDelimiter(2));
    +           assertEquals('.', registry.getDelimiter(3));
    +           assertEquals('.', registry.getDelimiter(-1));
    +
    +           registry.shutdown();
    +   }
    +
    +   @Test
    +   public void testConfigurableDelimiterForReportersInGroup() {
    +           Configuration config = new Configuration();
    +           config.setString(ConfigConstants.METRICS_REPORTERS_LIST, 
"test1,test2,test3,test4");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test1." + ConfigConstants.METRICS_REPORTER_SCOPE_DELIMITER, "_");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test1." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter8.class.getName());
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test2." + ConfigConstants.METRICS_REPORTER_SCOPE_DELIMITER, "-");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test2." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter8.class.getName());
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test3." + ConfigConstants.METRICS_REPORTER_SCOPE_DELIMITER, "AA");
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test3." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter8.class.getName());
    +           config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + 
"test4." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, 
TestReporter8.class.getName());
    +           config.setString(ConfigConstants.METRICS_SCOPE_NAMING_TM, 
"A.B");
    +
    +           MetricRegistry registry = new MetricRegistry(config);
    +           List<MetricReporter> reporters = registry.getReporters();
    +           ((TestReporter8)reporters.get(0)).expectedDelimiter = '_'; 
//test1  reporter
    +           ((TestReporter8)reporters.get(1)).expectedDelimiter = '-'; 
//test2 reporter
    +           ((TestReporter8)reporters.get(2)).expectedDelimiter = '.'; 
//test3 reporter, because 'AA' - not correct delimiter
    +           //for test4 reporter use global delimiter
    +
    --- End diff --
    
    are you sure that this test accurately detects errors? if the assert within 
notify() fails an exception is thrown, which however is never propagated but 
only logged.
    
    It would however be correct if you execute countCall++ after 
assertEquals(), as you only reach it for a correct delimiter. Then of course it 
should be renamed to something like "numCorrectDelimiters" or something.


> [metrics] Delimiter should be configured per reporter
> -----------------------------------------------------
>
>                 Key: FLINK-4564
>                 URL: https://issues.apache.org/jira/browse/FLINK-4564
>             Project: Flink
>          Issue Type: Bug
>          Components: Metrics
>    Affects Versions: 1.1.0
>            Reporter: Chesnay Schepler
>            Assignee: Anton Mushin
>
> Currently, the delimiter used or the scope string is based on a configuration 
> setting shared by all reporters. However, different reporters may have 
> different requirements in regards to the delimiter, as such we should allow 
> reporters to use a different delimiter.
> We can keep the current setting as a global setting that is used if no 
> specific setting was set.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to