HADOOP-19218. Addendum. Update TestFSNamesystemLockReport to exclude hostname resolution from regex. #6951
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jul 20, 2024
| @@ -8838,6 +8838,10 @@ private Supplier<String> getLockReportInfoSupplier(String src, String dst, | |||
| UserGroupInformation ugi = Server.getRemoteUser(); | |||
| String userName = ugi != null ? ugi.toString() : null; | |||
| InetAddress addr = Server.getRemoteIp(); | |||
| if (addr != null) { | |||
There was a problem hiding this comment.
@virajjasani Thanks for your works. But I am concerned if it will involve more serious impact here because when hold lock for a long time and trigger hostname resolution here which is one IO inner lock. Thanks again.
There was a problem hiding this comment.
hmm, FSNamesystemLock#coarseLock (writeLock) is unlocked only after writing above log so we will incur extra latency for long lock holding thread. Btw, i think coarseLock writeLock can be unlocked before logging the info as well (by protecting longestWriteLockHeldInfo with either synchronizing or using another lock), but this is out of scope here so maybe sometime later.
Shall we keep ip address only for long lock hold audit logs (similar to FSNamesystem audit logs)? If yes, then we can directly make test changes (i.e. revert TestFSNamesystemLockReport changes done with 88914ca)?
There was a problem hiding this comment.
Strong +1 to update TestFSNamesystemLockReport only. And move log out of lock could improve at next PR. Thanks.
There was a problem hiding this comment.
Sounds good, let me update the test. Thanks @Hexiaoqiao
4 tasks
zeekling
reviewed
Jul 20, 2024
| @@ -8838,6 +8838,10 @@ private Supplier<String> getLockReportInfoSupplier(String src, String dst, | |||
| UserGroupInformation ugi = Server.getRemoteUser(); | |||
| String userName = ugi != null ? ugi.toString() : null; | |||
| InetAddress addr = Server.getRemoteIp(); | |||
| if (addr != null) { | |||
| // The host name resolution on InetAddress provides Hostname with IP address | |||
| addr.getHostName(); | |||
There was a problem hiding this comment.
The parsing result is not used, are you sure it is useful?
There was a problem hiding this comment.
Yes it is useful because once you perform hostname resolution, the implementation of InetAddress#toString lets you print both hostname as well as ip address.
virajjasani
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
changed the title
Hexiaoqiao
pushed a commit
that referenced
this pull request
Jul 23, 2024
…hostname resolution from regex. (#6951). Contributed by Viraj Jasani. Signed-off-by: He Xiaoqiao <[email protected]> (cherry picked from commit e000cbf)
|
Committed to trunk and branch-3.4. Thanks @virajjasani . |
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
…hostname resolution from regex. (apache#6951). Contributed by Viraj Jasani. Signed-off-by: He Xiaoqiao <[email protected]>
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
…hostname resolution from regex. (apache#6951). Contributed by Viraj Jasani. Signed-off-by: He Xiaoqiao <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Addendum to fix hostname resolution for the longest lock holding thread
How was this patch tested?
UTs
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?