HDFS-16872. Fix log throttling by declaring LogThrottlingHelper as static members #5246
Conversation
|
Here is why I added the reset- Each test method in
This will break the previous way of testing log throttling, which uses a faked timer. Therefore this patch:
These may not be the best solution. Please take a look @xkrogen . Thanks. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
So one issue I realized with this approach is that LogThrottlingHelper isn't thread-safe. Is it possible to have multiple instances of FSEditLogLoader or RedundantEditLogInputStream running simultaneously? I think yes?
We probably need to make LogThrottlingHelper properly threadsafe (atomics, concurrent hash map, etc.). I considered using a ThreadLocal within the callers, for simplicity, but it doesn't work since we do want the throttling to be across threads.
| if (currentTimeMs < lastLogTimestampMs) { | ||
| // Reset lastLogTimestampMs: this should only happen in tests | ||
| lastLogTimestampMs = Long.MIN_VALUE; | ||
| } |
There was a problem hiding this 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?
There was a problem hiding this 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?
There was a problem hiding this 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?
There was a problem hiding this comment.
Nice solution. I will fix this.
IMO, to make It seems that the only way to make it thread-safe is to add
Tracing down usages of |
f730bdd to
71f58f7
Compare
|
💔 -1 overall
This message was automatically generated. |
|
I spent a little time poking at |
I now see what you mean. Thank you for the demo. This looks a bit too complex and unreadable.
Yes, I think we should just use I will upload a new version soon. |
71f58f7 to
4563893
Compare
|
💔 -1 overall
This message was automatically generated. |
|
The Javadoc failures on JDK 11 and the TestLeaseRecovery2 failure are both known issues. |
|
Also backported to branch-3.3 and branch-3.2 |
|
Thanks @xkrogen for review and committing! |
Description of PR
In our production cluster with Observer NameNode enabled, we have plenty of logs printed by
FSEditLogLoaderandRedundantEditLogInputStream. TheLogThrottlingHelperdoesn't seem to work.After some digging, I found the cause is that
LogThrottlingHelper's are declared as instance variables of all the enclosing classes, includingFSImage,FSEditLogLoaderandRedundantEditLogInputStream. Therefore the logging frequency will not be limited across different instances. For classes with only limited number of instances, such asFSImage, this is fine. For others whose instances are created frequently, such asFSEditLogLoaderandRedundantEditLogInputStream, 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.