Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-11301

[optionally] update jmx cache to drop old metrics

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.7.0, 2.6.5
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      MetricsSourceAdapter::updateJmxCache() skips updating the info cache if no new metric is added since last time:

            int oldCacheSize = attrCache.size();
            int newCacheSize = updateAttrCache();
            if (oldCacheSize < newCacheSize) {
              updateInfoCache();
            }
      

      This behavior is not desirable in some applications. For example nntop (HDFS-6982) reports the top users via jmx. The list is updated after each report. The previously reported top users hence should be removed from the cache upon each report request.

      In our production run of nntop we made a change to ignore the size check and always perform updateInfoCache. I am planning to submit a patch including this change. The feature can be enabled by a configuration parameter.

      1. HADOOP-11301.v04.patch
        4 kB
        Maysam Yabandeh
      2. HADOOP-11301.v03.patch
        4 kB
        Maysam Yabandeh
      3. HADOOP-11301.v02.patch
        4 kB
        Maysam Yabandeh
      4. HADOOP-11301.v01.patch
        7 kB
        Maysam Yabandeh

        Issue Links

          Activity

          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          Andrew Wang, I am attaching the initial patch that demonstrates the problem and offers a basic solution that we employed in nntop.

          Show
          maysamyabandeh Maysam Yabandeh added a comment - Andrew Wang , I am attaching the initial patch that demonstrates the problem and offers a basic solution that we employed in nntop.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12682047/HADOOP-11301.v01.patch
          against trunk revision 2fce6d6.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5093//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5093//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12682047/HADOOP-11301.v01.patch against trunk revision 2fce6d6. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5093//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5093//console This message is automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hey Maysam, thanks for working on this! A few comments:

          • Rather than adding a config parameter, I think we can just do this always. The cost of determining the MBeanInfo seems small compared to gathering all the metrics, and that you run it in production is evidence in favor of this.
          • It's our coding style to avoid wildcard imports, there's one in the new test.
          Show
          andrew.wang Andrew Wang added a comment - Hey Maysam, thanks for working on this! A few comments: Rather than adding a config parameter, I think we can just do this always. The cost of determining the MBeanInfo seems small compared to gathering all the metrics, and that you run it in production is evidence in favor of this. It's our coding style to avoid wildcard imports, there's one in the new test.
          Hide
          stack stack added a comment -

          Agree with Andrew Wang. No need of a config. Can you just update the info bean if getAllMetrics is true otherwise leave the old bean in place? Thanks.

          Show
          stack stack added a comment - Agree with Andrew Wang . No need of a config. Can you just update the info bean if getAllMetrics is true otherwise leave the old bean in place? Thanks.
          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          Thanks Andrew Wang and stack. I am attaching the new patch that updates info cache whenever getAllMetrics is true.

          Show
          maysamyabandeh Maysam Yabandeh added a comment - Thanks Andrew Wang and stack . I am attaching the new patch that updates info cache whenever getAllMetrics is true.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12682948/HADOOP-11301.v02.patch
          against trunk revision 23dacb3.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5111//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5111//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12682948/HADOOP-11301.v02.patch against trunk revision 23dacb3. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5111//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5111//console This message is automatically generated.
          Hide
          stack stack added a comment -

          Looks much cleaner. Should these two statements be swapped?

          176	      updateAttrCache();
          177	      if (getAllMetrics) {
          

          ... so we only updateAttrCache if getAllMetrics is true?

          Show
          stack stack added a comment - Looks much cleaner. Should these two statements be swapped? 176 updateAttrCache(); 177 if (getAllMetrics) { ... so we only updateAttrCache if getAllMetrics is true?
          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          Thanks stack, makes sense. Previously was called all the time to figure out if the cache size is changed or not.

                int newCacheSize = updateAttrCache();
                if (oldCacheSize < newCacheSize) {
                  updateInfoCache();
                }
          

          But since now the update to info cache is only based on getAllMetrics value, it makes sense to update attr cache only when info cache is updated. I am updating a new patch that includes this change.

          Show
          maysamyabandeh Maysam Yabandeh added a comment - Thanks stack , makes sense. Previously was called all the time to figure out if the cache size is changed or not. int newCacheSize = updateAttrCache(); if (oldCacheSize < newCacheSize) { updateInfoCache(); } But since now the update to info cache is only based on getAllMetrics value, it makes sense to update attr cache only when info cache is updated. I am updating a new patch that includes this change.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12682976/HADOOP-11301.v03.patch
          against trunk revision 233b61e.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.metrics2.impl.TestMetricsSourceAdapter

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5112//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5112//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12682976/HADOOP-11301.v03.patch against trunk revision 233b61e. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.metrics2.impl.TestMetricsSourceAdapter +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5112//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5112//console This message is automatically generated.
          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          stack, this test requires updateAttrCache to be invoked upon each updateJmxCache call.

            public void testGetMetricsAndJmx() throws Exception {
          ...
              Thread.sleep(100); // skip JMX cache TTL
              assertEquals(0L, (Number)sa.getAttribute("C1"));
          

          At this point I am not sure whether the rest of hadoop depends on this or the unit test demands are broader than what the users of jmx actually need. A safer option is to preserve backward compatibility and invoke updateAttrCache all the time. Let me know your thoughts on this.

          Show
          maysamyabandeh Maysam Yabandeh added a comment - stack , this test requires updateAttrCache to be invoked upon each updateJmxCache call. public void testGetMetricsAndJmx() throws Exception { ... Thread .sleep(100); // skip JMX cache TTL assertEquals(0L, ( Number )sa.getAttribute( "C1" )); At this point I am not sure whether the rest of hadoop depends on this or the unit test demands are broader than what the users of jmx actually need. A safer option is to preserve backward compatibility and invoke updateAttrCache all the time. Let me know your thoughts on this.
          Hide
          stack stack added a comment -

          Lets go with your conservative suggestion. It will make the test pass but in practice we will return early when updateJmxCache is called if jmxCacheTTL has not yet elapsed. Is that how you read it Maysam Yabandeh? Thanks.

          Show
          stack stack added a comment - Lets go with your conservative suggestion. It will make the test pass but in practice we will return early when updateJmxCache is called if jmxCacheTTL has not yet elapsed. Is that how you read it Maysam Yabandeh ? Thanks.
          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          Sure. I have to admit the the logic behind updateJmxCache is a bit confusing for me and it is not very clear for me that when the cache was intended to be updated. To reconstruct the expected contract, which is missing in the code, one would need to review all the usages inside the hadoop project. It seems that jmx meant to update the metrics with a frequency less then or equal to 2*jmxCacheTTL.

          As you suggested I will upload a patch with the conservative approach of keeping the current implementation backward compatible with previous unit tests.

          Show
          maysamyabandeh Maysam Yabandeh added a comment - Sure. I have to admit the the logic behind updateJmxCache is a bit confusing for me and it is not very clear for me that when the cache was intended to be updated. To reconstruct the expected contract, which is missing in the code, one would need to review all the usages inside the hadoop project. It seems that jmx meant to update the metrics with a frequency less then or equal to 2*jmxCacheTTL. As you suggested I will upload a patch with the conservative approach of keeping the current implementation backward compatible with previous unit tests.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12683002/HADOOP-11301.v04.patch
          against trunk revision a128cca.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverControllerStress

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5113//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5113//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12683002/HADOOP-11301.v04.patch against trunk revision a128cca. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverControllerStress +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5113//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5113//console This message is automatically generated.
          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          the failed unit test seems unrelated.

          Show
          maysamyabandeh Maysam Yabandeh added a comment - the failed unit test seems unrelated.
          Hide
          stack stack added a comment -

          I tried the patch and it seems like it is doing as expected, Maysam Yabandeh.

          My test is basic. I just invoke the /jmx page in the UI. Before the patch, with this simple logging inserted:

          @@ -175,7 +175,9 @@ private void updateJmxCache() {
               synchronized(this) {
                 int oldCacheSize = attrCache.size();
                 int newCacheSize = updateAttrCache();
          +      LOG.info("Called updateAttrCache");
                 if (oldCacheSize < newCacheSize) {
          +        LOG.info("Called updateInfoCache");
                   updateInfoCache();
                 }
                 jmxCacheTS = Time.now();
          

          ... I would see this output on each request in unpatched case.

          2014-11-24 22:44:41,688 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,689 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,693 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,694 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,694 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,694 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 22:44:41,696 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,699 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 22:44:41,699 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 22:44:41,699 INFO  [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache
          

          I then applied the patch plus logging and would get this output on each page invocation:

          2014-11-24 23:20:49,145 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,145 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,146 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,146 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,154 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,154 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,155 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,156 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,157 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,157 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,193 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,193 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,203 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,203 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:20:49,203 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache
          2014-11-24 23:20:49,203 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          

          Its same amount of updateAttrCache updates but we are now doing a call to updateInfoCache for each updateAttrCache call. Thats as expected I believe.

          I was thinking that we were refreshing attrs and the bean on each invocation but added this:

          @@ -163,6 +163,7 @@ private void updateJmxCache() {
                   }
                 }
                 else {
          +        LOG.info("Returned w/o updateAttrCache");
                   return;
                 }
               }
          

          ... and then see this:

          2014-11-24 23:45:22,002 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,002 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,002 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,002 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,002 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,002 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache ttl=11, all=true
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,005 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache
          2014-11-24 23:45:22,047 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache ttl=11, all=true
          2014-11-24 23:45:22,047 INFO  [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache
          ...
          

          So we are using cached data sometimes. Default says use cache for ten seconds. That is not what I see above ('11'?). I should look at this more.

          Show
          stack stack added a comment - I tried the patch and it seems like it is doing as expected, Maysam Yabandeh . My test is basic. I just invoke the /jmx page in the UI. Before the patch, with this simple logging inserted: @@ -175,7 +175,9 @@ private void updateJmxCache() { synchronized ( this ) { int oldCacheSize = attrCache.size(); int newCacheSize = updateAttrCache(); + LOG.info( "Called updateAttrCache" ); if (oldCacheSize < newCacheSize) { + LOG.info( "Called updateInfoCache" ); updateInfoCache(); } jmxCacheTS = Time.now(); ... I would see this output on each request in unpatched case. 2014-11-24 22:44:41,688 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,689 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,693 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,694 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,694 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,694 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 22:44:41,696 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,699 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 22:44:41,699 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 22:44:41,699 INFO [1851255134@qtp-1525844775-1] impl.MetricsSourceAdapter: Called updateAttrCache I then applied the patch plus logging and would get this output on each page invocation: 2014-11-24 23:20:49,145 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,145 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,146 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,146 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,154 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,154 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,155 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,156 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,157 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,157 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,193 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,193 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,203 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,203 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:20:49,203 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache 2014-11-24 23:20:49,203 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache Its same amount of updateAttrCache updates but we are now doing a call to updateInfoCache for each updateAttrCache call. Thats as expected I believe. I was thinking that we were refreshing attrs and the bean on each invocation but added this: @@ -163,6 +163,7 @@ private void updateJmxCache() { } } else { + LOG.info( "Returned w/o updateAttrCache" ); return ; } } ... and then see this: 2014-11-24 23:45:22,002 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,002 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,002 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,002 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,002 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,002 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache ttl=11, all= true 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,005 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Returned w/o updateAttrCache 2014-11-24 23:45:22,047 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateAttrCache ttl=11, all= true 2014-11-24 23:45:22,047 INFO [2113243119@qtp-1525844775-0] impl.MetricsSourceAdapter: Called updateInfoCache ... So we are using cached data sometimes. Default says use cache for ten seconds. That is not what I see above ('11'?). I should look at this more.
          Hide
          stack stack added a comment -

          We cache jmx values for 10ms only. Too short. The value passed in here is value of the overloaded PERIOD_KEY, which is DEFAULT_PERIOD: '10' (says 10 'seconds' but whats passed in here is '10'). TODO is a config explicitly about jmx cache time, not a reuse of the general PERIOD_KEY. As is, this patch has us doing a little more work – each invocation of updateJMXCache beyond 10ms has us doing this: infoCache = infoBuilder.reset(lastRecs).get() (lastRec was being recalculated regardless even before this patch) – but thats the point, jmx stats were stale. I'm +1 on the patch. Will commit in a day unless objection.

          Show
          stack stack added a comment - We cache jmx values for 10ms only. Too short. The value passed in here is value of the overloaded PERIOD_KEY, which is DEFAULT_PERIOD: '10' (says 10 'seconds' but whats passed in here is '10'). TODO is a config explicitly about jmx cache time, not a reuse of the general PERIOD_KEY. As is, this patch has us doing a little more work – each invocation of updateJMXCache beyond 10ms has us doing this: infoCache = infoBuilder.reset(lastRecs).get() (lastRec was being recalculated regardless even before this patch) – but thats the point, jmx stats were stale. I'm +1 on the patch. Will commit in a day unless objection.
          Hide
          ram_krish ramkrishna.s.vasudevan added a comment -

          The commit msg was added as HBASE-11301I I think. JFYI.

          Show
          ram_krish ramkrishna.s.vasudevan added a comment - The commit msg was added as HBASE-11301 I I think. JFYI.
          Hide
          stack stack added a comment -

          Thanks ramkrishna.s.vasudevan Fixed.

          Show
          stack stack added a comment - Thanks ramkrishna.s.vasudevan Fixed.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6629 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6629/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6629 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6629/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          stack stack added a comment -

          Pushed to branch-2 and trunk (only took me three attempts). Thanks for the patch Maysam Yabandeh

          Show
          stack stack added a comment - Pushed to branch-2 and trunk (only took me three attempts). Thanks for the patch Maysam Yabandeh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #23 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/23/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #23 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/23/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          ABORTED: Integrated in Hadoop-Hdfs-trunk #1951 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1951/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          Show
          hudson Hudson added a comment - ABORTED: Integrated in Hadoop-Hdfs-trunk #1951 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1951/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #761 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/761/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #761 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/761/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #23 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/23/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #23 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/23/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1975 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1975/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1975 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1975/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk-Java8 #23 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/23/)
          HADOOP-11301. [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5)

          • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk-Java8 #23 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/23/ ) HADOOP-11301 . [optionally] update jmx cache to drop old metrics (Maysam Yabandeh via stack) – REAPPLY #2 (stack: rev b36f29298247f6c5db9f6eff7c2f5177a0fe3de5) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestMetricsSourceAdapter.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
          Hide
          maysamyabandeh Maysam Yabandeh added a comment -

          Thanks stack for the review and the commit(s).

          Show
          maysamyabandeh Maysam Yabandeh added a comment - Thanks stack for the review and the commit(s).
          Hide
          stack stack added a comment -

          Just to say that this patch also fixes an issue in hbase where when a region is deleted (split or table removed), metrics would get stuck reporting the last state of metrics on the removed region; metrics was showing 'ghosts'. Thanks Maysam Yabandeh

          Show
          stack stack added a comment - Just to say that this patch also fixes an issue in hbase where when a region is deleted (split or table removed), metrics would get stuck reporting the last state of metrics on the removed region; metrics was showing 'ghosts'. Thanks Maysam Yabandeh
          Hide
          sjlee0 Sangjin Lee added a comment -

          Cherry-picked to 2.6.5 (trivial).

          Show
          sjlee0 Sangjin Lee added a comment - Cherry-picked to 2.6.5 (trivial).

            People

            • Assignee:
              maysamyabandeh Maysam Yabandeh
              Reporter:
              maysamyabandeh Maysam Yabandeh
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development