You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/12/20 05:21:02 UTC
[GitHub] [hadoop] ChengbingLiu opened a new pull request, #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
ChengbingLiu opened a new pull request, #5246:
URL: https://github.com/apache/hadoop/pull/5246
### Description of PR
In our production cluster with Observer NameNode enabled, we have plenty of logs printed by `FSEditLogLoader` and `RedundantEditLogInputStream`. The `LogThrottlingHelper` doesn't seem to work.
```
2022-10-25 09:26:50,380 INFO org.apache.hadoop.hdfs.server.namenode.FSImage: Start loading edits file ByteStringEditLog[17686250688, 17686250688], ByteStringEditLog[17686250688, 17686250688], ByteStringEditLog[17686250688, 17686250688] maxTxnsToRead = 92233720368547758072022-10-25 09:26:50,380 INFO org.apache.hadoop.hdfs.server.namenode.RedundantEditLogInputStream: Fast-forwarding stream 'ByteStringEditLog[17686250688, 17686250688], ByteStringEditLog[17686250688, 17686250688], ByteStringEditLog[17686250688, 17686250688]' to transaction ID 17686250688
2022-10-25 09:26:50,380 INFO org.apache.hadoop.hdfs.server.namenode.RedundantEditLogInputStream: Fast-forwarding stream 'ByteStringEditLog[17686250688, 17686250688]' to transaction ID 17686250688
2022-10-25 09:26:50,380 INFO org.apache.hadoop.hdfs.server.namenode.FSImage: Loaded 1 edits file(s) (the last named ByteStringEditLog[17686250688, 17686250688], ByteStringEditLog[17686250688, 17686250688], ByteStringEditLog[17686250688, 17686250688]) of total size 527.0, total edits 1.0, total load time 0.0 ms
2022-10-25 09:26:50,387 INFO org.apache.hadoop.hdfs.server.namenode.FSImage: Start loading edits file ByteStringEditLog[17686250689, 17686250693], ByteStringEditLog[17686250689, 17686250693], ByteStringEditLog[17686250689, 17686250693] maxTxnsToRead = 9223372036854775807
2022-10-25 09:26:50,387 INFO org.apache.hadoop.hdfs.server.namenode.RedundantEditLogInputStream: Fast-forwarding stream 'ByteStringEditLog[17686250689, 17686250693], ByteStringEditLog[17686250689, 17686250693], ByteStringEditLog[17686250689, 17686250693]' to transaction ID 17686250689
2022-10-25 09:26:50,387 INFO org.apache.hadoop.hdfs.server.namenode.RedundantEditLogInputStream: Fast-forwarding stream 'ByteStringEditLog[17686250689, 17686250693]' to transaction ID 17686250689
2022-10-25 09:26:50,387 INFO org.apache.hadoop.hdfs.server.namenode.FSImage: Loaded 1 edits file(s) (the last named ByteStringEditLog[17686250689, 17686250693], ByteStringEditLog[17686250689, 17686250693], ByteStringEditLog[17686250689, 17686250693]) of total size 890.0, total edits 5.0, total load time 1.0 ms
```
After some digging, I found the cause is that `LogThrottlingHelper`'s are declared as instance variables of all the enclosing classes, including `FSImage`, `FSEditLogLoader` and `RedundantEditLogInputStream`. Therefore the logging frequency will not be limited across different instances. For classes with only limited number of instances, such as `FSImage`, this is fine. For others whose instances are created frequently, such as `FSEditLogLoader` and `RedundantEditLogInputStream`, it will result in plenty of logs.
This can be fixed by declaring `LogThrottlingHelper`'s as static members.
### How was this patch tested?
Through a test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ChengbingLiu commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
ChengbingLiu commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1362517793
> So one issue I realized with this approach is that `LogThrottlingHelper` isn't thread-safe.
We probably need to make `LogThrottlingHelper` properly threadsafe (atomics, concurrent hash map, etc.).
IMO, to make `LogThrottlingHelper` thread-safe, it isn't enough just to make its member variables thread-safe. For example, if we just change `Map<String, LoggingAction> currentLogs;` to a ConcurrentHashMap, it won't help because there are multiple gets and puts inside the `record` method.
It seems that the only way to make it thread-safe is to add `synchronized` to `record(...)` as well as other methods. However, doing so may affect the performance.
> Is it possible to have multiple instances of `FSEditLogLoader` or `RedundantEditLogInputStream` running simultaneously? I think yes?
Tracing down usages of `LogThrottlingHelper` in `FSEditLogLoader` or `RedundantEditLogInputStream`, the main usage seems to happen inside a `FSNameSystem` writeLock/writeUnlock block, yet there might be other cases that can have a concurrency issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ChengbingLiu commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
ChengbingLiu commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1376680930
> I spent a little time poking at `LogThrottlingHelper` to see what it would take to make it concurrent. I have a demonstration PR here: #5269. It's a bit complicated as it relies on functionality from `ConcurrentHashMap` as well as some atomic operations (mostly CAS).
I now see what you mean. Thank you for the demo. This looks a bit too complex and unreadable.
> I suspect it's likely overkill from an implementation complexity perspective since we don't _expect_ high contention on this class. I would say it probably makes sense to just use `synchronized`. WDYT?
Yes, I think we should just use `synchronized`. In our production cluster, the call to `record(...)` happens about every millisecond (quite frequent already). According to _Java Performance, 2nd edition_ by Scott Oaks, "the cost of obtaining an uninflated lock is on the order of a few hundred nanoseconds", which is far less than the frequency. So it will not add too much performance penalty to use `synchronized`.
I will upload a new version soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1377680930
Also backported to branch-3.3 and branch-3.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1368051863
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 23s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 15m 12s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 29m 4s | | trunk passed |
| +1 :green_heart: | compile | 25m 56s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | compile | 22m 21s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | checkstyle | 4m 10s | | trunk passed |
| +1 :green_heart: | mvnsite | 3m 19s | | trunk passed |
| -1 :x: | javadoc | 1m 10s | [/branch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/2/artifact/out/branch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt) | hadoop-common in trunk failed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04. |
| +1 :green_heart: | javadoc | 2m 24s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 7m 27s | | trunk passed |
| +1 :green_heart: | shadedclient | 27m 16s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 26s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 26s | | the patch passed |
| +1 :green_heart: | compile | 24m 49s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javac | 24m 49s | | the patch passed |
| +1 :green_heart: | compile | 22m 5s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | javac | 22m 5s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 3m 56s | | the patch passed |
| +1 :green_heart: | mvnsite | 3m 19s | | the patch passed |
| -1 :x: | javadoc | 1m 2s | [/patch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/2/artifact/out/patch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt) | hadoop-common in the patch failed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04. |
| +1 :green_heart: | javadoc | 2m 21s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 6m 26s | | the patch passed |
| +1 :green_heart: | shadedclient | 26m 19s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 13s | | hadoop-common in the patch passed. |
| -1 :x: | unit | 473m 21s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/2/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. |
| +1 :green_heart: | asflicense | 1m 3s | | The patch does not generate ASF License warnings. |
| | | 725m 28s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hdfs.server.namenode.ha.TestObserverNode |
| | hadoop.hdfs.TestLeaseRecovery2 |
| | hadoop.hdfs.server.blockmanagement.TestBlockTokenWithShortCircuitRead |
| | hadoop.hdfs.server.balancer.TestBalancerWithHANameNodes |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5246 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 10c21b94c0ae 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24 18:16:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 71f58f7ff9860926f8f830cf8e5ac47b0c1b3cb1 |
| Default Java | Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/2/testReport/ |
| Max. process+thread count | 2234 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/2/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ChengbingLiu commented on a diff in pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
ChengbingLiu commented on code in PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#discussion_r1054026139
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogThrottlingHelper.java:
##########
@@ -259,6 +259,10 @@ public LogAction record(String recorderName, long currentTimeMs,
currentLogs.put(recorderName, currentLog);
}
currentLog.recordValues(values);
+ if (currentTimeMs < lastLogTimestampMs) {
+ // Reset lastLogTimestampMs: this should only happen in tests
+ lastLogTimestampMs = Long.MIN_VALUE;
+ }
Review Comment:
I thought about this solution, but the problem is that we have to add `reset()` methods in `LogThrottlingHelper` as well as `FSEditLogLoader`. Will this be OK?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1377455449
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 4s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 15m 48s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 34m 8s | | trunk passed |
| +1 :green_heart: | compile | 25m 14s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | compile | 21m 48s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | checkstyle | 4m 5s | | trunk passed |
| +1 :green_heart: | mvnsite | 3m 15s | | trunk passed |
| -1 :x: | javadoc | 1m 5s | [/branch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/3/artifact/out/branch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt) | hadoop-common in trunk failed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04. |
| +1 :green_heart: | javadoc | 2m 22s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 6m 13s | | trunk passed |
| +1 :green_heart: | shadedclient | 29m 9s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 24s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 35s | | the patch passed |
| +1 :green_heart: | compile | 24m 40s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javac | 24m 40s | | the patch passed |
| +1 :green_heart: | compile | 21m 53s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | javac | 21m 53s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 3m 54s | | the patch passed |
| +1 :green_heart: | mvnsite | 3m 12s | | the patch passed |
| -1 :x: | javadoc | 1m 0s | [/patch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/3/artifact/out/patch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt) | hadoop-common in the patch failed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04. |
| +1 :green_heart: | javadoc | 2m 22s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 6m 19s | | the patch passed |
| +1 :green_heart: | shadedclient | 29m 29s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 6s | | hadoop-common in the patch passed. |
| -1 :x: | unit | 453m 53s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/3/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. |
| +1 :green_heart: | asflicense | 1m 6s | | The patch does not generate ASF License warnings. |
| | | 713m 49s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hdfs.server.namenode.ha.TestObserverNode |
| | hadoop.hdfs.TestLeaseRecovery2 |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5246 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux b17f84542ae2 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24 18:16:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 4563893e1bc7c458dcd599b82704b1e5cb7bc58d |
| Default Java | Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/3/testReport/ |
| Max. process+thread count | 2530 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/3/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1360046069
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 2m 45s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 15m 13s | | Maven dependency ordering for branch |
| -1 :x: | mvninstall | 17m 43s | [/branch-mvninstall-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/artifact/out/branch-mvninstall-root.txt) | root in trunk failed. |
| +1 :green_heart: | compile | 28m 56s | | trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | compile | 25m 44s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | checkstyle | 4m 30s | | trunk passed |
| +1 :green_heart: | mvnsite | 3m 56s | | trunk passed |
| -1 :x: | javadoc | 1m 16s | [/branch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/artifact/out/branch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt) | hadoop-common in trunk failed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04. |
| +1 :green_heart: | javadoc | 2m 38s | | trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 6m 32s | | trunk passed |
| +1 :green_heart: | shadedclient | 27m 14s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 27s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 30s | | the patch passed |
| +1 :green_heart: | compile | 26m 21s | | the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 |
| +1 :green_heart: | javac | 26m 21s | | the patch passed |
| +1 :green_heart: | compile | 24m 36s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | javac | 24m 36s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 5m 35s | | the patch passed |
| +1 :green_heart: | mvnsite | 5m 8s | | the patch passed |
| -1 :x: | javadoc | 1m 9s | [/patch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/artifact/out/patch-javadoc-hadoop-common-project_hadoop-common-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt) | hadoop-common in the patch failed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04. |
| +1 :green_heart: | javadoc | 2m 32s | | the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| +1 :green_heart: | spotbugs | 7m 12s | | the patch passed |
| +1 :green_heart: | shadedclient | 24m 9s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 35s | | hadoop-common in the patch passed. |
| -1 :x: | unit | 578m 54s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. |
| +1 :green_heart: | asflicense | 1m 27s | | The patch does not generate ASF License warnings. |
| | | 836m 51s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hdfs.TestReconstructStripedFileWithValidator |
| | hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped |
| | hadoop.hdfs.TestLeaseRecovery2 |
| | hadoop.hdfs.server.diskbalancer.command.TestDiskBalancerCommand |
| | hadoop.hdfs.server.namenode.ha.TestSeveralNameNodes |
| | hadoop.hdfs.server.balancer.TestBalancerWithHANameNodes |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5246 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 52e1e57618d4 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24 18:16:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / f730bddc5f4071c7b7cba4a07e4cefcc75154aed |
| Default Java | Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_352-8u352-ga-1~20.04-b08 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/testReport/ |
| Max. process+thread count | 2462 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5246/1/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ChengbingLiu commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
ChengbingLiu commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1378048406
Thanks @xkrogen for review and committing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen commented on a diff in pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen commented on code in PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#discussion_r1054988970
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogThrottlingHelper.java:
##########
@@ -259,6 +259,10 @@ public LogAction record(String recorderName, long currentTimeMs,
currentLogs.put(recorderName, currentLog);
}
currentLog.recordValues(values);
+ if (currentTimeMs < lastLogTimestampMs) {
+ // Reset lastLogTimestampMs: this should only happen in tests
+ lastLogTimestampMs = Long.MIN_VALUE;
+ }
Review Comment:
Why does `FSEditLogLoader` need a reset method? Just so we can trigger the reset from within `TestFSEditLogLoader`? If so then can we just made `FSEditLogLoader.LOAD_EDITS_LOG_HELPER` package-private so that we can access it from within `TestFSEditLogLoader` and then call `reset()` directly on the `LogThrottlingHelper`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ChengbingLiu commented on a diff in pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
ChengbingLiu commented on code in PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#discussion_r1055162289
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogThrottlingHelper.java:
##########
@@ -259,6 +259,10 @@ public LogAction record(String recorderName, long currentTimeMs,
currentLogs.put(recorderName, currentLog);
}
currentLog.recordValues(values);
+ if (currentTimeMs < lastLogTimestampMs) {
+ // Reset lastLogTimestampMs: this should only happen in tests
+ lastLogTimestampMs = Long.MIN_VALUE;
+ }
Review Comment:
Nice solution. I will fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen commented on a diff in pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen commented on code in PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#discussion_r1053850133
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogThrottlingHelper.java:
##########
@@ -259,6 +259,10 @@ public LogAction record(String recorderName, long currentTimeMs,
currentLogs.put(recorderName, currentLog);
}
currentLog.recordValues(values);
+ if (currentTimeMs < lastLogTimestampMs) {
+ // Reset lastLogTimestampMs: this should only happen in tests
+ lastLogTimestampMs = Long.MIN_VALUE;
+ }
Review Comment:
This is a bit awkward. How about instead we add a new method like `reset()` that clears all of the state and call this in the `beforeEach()` for the test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen commented on a diff in pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen commented on code in PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#discussion_r1053850133
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogThrottlingHelper.java:
##########
@@ -259,6 +259,10 @@ public LogAction record(String recorderName, long currentTimeMs,
currentLogs.put(recorderName, currentLog);
}
currentLog.recordValues(values);
+ if (currentTimeMs < lastLogTimestampMs) {
+ // Reset lastLogTimestampMs: this should only happen in tests
+ lastLogTimestampMs = Long.MIN_VALUE;
+ }
Review Comment:
This is a bit awkward. How about instead we add a new method like `reset()` that clears all of the state?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ChengbingLiu commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
ChengbingLiu commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1358874741
Here is why I added the reset-`lastLogTimestampMs` logic and slightly tweaked the test case:
Each test method in `TestFSEditLogLoader` is executed twice (due to the `@Parameterized` annotation of the class). Since I change the `LogThrottlingHelper` to a static field, other test method execution can have two effects:
1. set the `lastLogTimestampMs` field to a current time;
2. create `LoggingAction`s with suppressed logs.
This will break the previous way of testing log throttling, which uses a faked timer. Therefore this patch:
1. resets `lastLogTimestampMs` if the `currentTimeMs` is smaller (can only happen in test cases)
2. clears previous logs by a `loadFSEdits` call before the original test cases
These may not be the best solution. Please take a look @xkrogen . Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1370183767
I spent a little time poking at `LogThrottlingHelper` to see what it would take to make it concurrent. I have a demonstration PR here: #5269. It's a bit complicated as it relies on functionality from `ConcurrentHashMap` as well as some atomic operations (mostly CAS). I suspect it's likely overkill from an implementation complexity perspective since we don't _expect_ high contention on this class. I would say it probably makes sense to just use `synchronized`. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen commented on pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen commented on PR #5246:
URL: https://github.com/apache/hadoop/pull/5246#issuecomment-1377649280
The Javadoc failures on JDK 11 and the TestLeaseRecovery2 failure are both known issues. `TestObserverNode` failure looks unrelated and I wasn't able to reproduce it locally. Merging to trunk. Thanks @ChengbingLiu !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] xkrogen merged pull request #5246: HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members
Posted by GitBox <gi...@apache.org>.
xkrogen merged PR #5246:
URL: https://github.com/apache/hadoop/pull/5246
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org