[ https://issues.apache.org/jira/browse/HADOOP-18567?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17646903#comment-17646903 ]
ASF GitHub Bot commented on HADOOP-18567: ----------------------------------------- ChengbingLiu commented on code in PR #5215: URL: https://github.com/apache/hadoop/pull/5215#discussion_r1047964506 ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogThrottlingHelper.java: ########## @@ -262,8 +263,15 @@ public LogAction record(String recorderName, long currentTimeMs, if (primaryRecorderName.equals(recorderName) && currentTimeMs - minLogPeriodMs >= lastLogTimestampMs) { lastLogTimestampMs = currentTimeMs; - for (LoggingAction log : currentLogs.values()) { - log.setShouldLog(); + for (Iterator<LoggingAction> it = currentLogs.values().iterator(); it + .hasNext();) { + LoggingAction log = it.next(); + if (log.hasLogged()) { + // Make sure the dependent recorders will be triggered the next time + it.remove(); + } else { + log.setShouldLog(); + } } Review Comment: Makes sense. Added a new commit according to your suggestions. > LogThrottlingHelper: the dependent recorder is not triggered correctly > ---------------------------------------------------------------------- > > Key: HADOOP-18567 > URL: https://issues.apache.org/jira/browse/HADOOP-18567 > Project: Hadoop Common > Issue Type: Bug > Affects Versions: 3.3.4 > Reporter: Chengbing Liu > Priority: Major > Labels: pull-request-available > > The current implementation of {{LogThrottlingHelper}} works well most of the > time, but it missed out one case, which appears quite common in the > production codes: > - if the dependent recorder was not suppressed before the primary one is > triggered on the next period, then the next logging of the dependent recorder > will be unexpectedly suppressed. > {code:java} > helper = new LogThrottlingHelper(LOG_PERIOD, "foo", timer); > assertTrue(helper.record("foo", 0).shouldLog()); > assertTrue(helper.record("bar", 0).shouldLog()); > // Both should log once the period has elapsed > // <pos1> > assertTrue(helper.record("foo", LOG_PERIOD).shouldLog()); > assertTrue(helper.record("bar", LOG_PERIOD).shouldLog()); <--- This > assertion will now fail > {code} > Note if we call {{helper.record("bar", LOG_PERIOD * 2)}} in <pos1>, as the > existing test cases do, it will work as expected. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org