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. |
xkrogen
left a comment
xkrogen
left a comment
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 |
ChengbingLiu
force-pushed
the
HDFS-16872
branch
from
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. |
ChengbingLiu
force-pushed
the
HDFS-16872
branch
from
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. |
…atic members (#5246) Co-authored-by: Chengbing Liu <[email protected]> Signed-off-by: Erik Krogen <[email protected]> (cherry picked from commit 4cf304d)
…atic members (#5246) Co-authored-by: Chengbing Liu <[email protected]> Signed-off-by: Erik Krogen <[email protected]> (cherry picked from commit 4cf304d) (cherry picked from commit af96e0f)
|
Also backported to branch-3.3 and branch-3.2 |
|
Thanks @xkrogen for review and committing! |
3 participants
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.