HADOOP-18014. CallerContext should not include some characters. #3698
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 task
virajjasani
reviewed
Nov 22, 2021
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Left one minor nit. The core logic of using escapeJava() to parse the special chars seems the right choice here.
Comment on lines
608
to
615
| MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build(); | ||
| LogCapturer auditlog = LogCapturer.captureLogs(FSNamesystem.auditLog); | ||
|
|
||
| try { | ||
| cluster.waitClusterUp(); | ||
| final FileSystem fs = cluster.getFileSystem(); | ||
| final long time = System.currentTimeMillis(); | ||
| final Path p = new Path("/"); |
Contributor
There was a problem hiding this comment.
nit: how about using try-with-resources here?
try (MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build()) {
LogCapturer auditlog = LogCapturer.captureLogs(FSNamesystem.auditLog);
cluster.waitClusterUp();
...
...
...
}
|
💔 -1 overall
This message was automatically generated. |
virajjasani
reviewed
Nov 22, 2021
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Seems we might have to consider few things for test, else the core changes look good
| final long time = System.currentTimeMillis(); | ||
| final Path p = new Path("/"); | ||
|
|
||
| assertNull(CallerContext.getCurrent()); |
Contributor
There was a problem hiding this comment.
This seems to be failing as per QA report:
Error Message
expected null, but was:<>
Stacktrace
java.lang.AssertionError: expected null, but was:<>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotNull(Assert.java:756)
at org.junit.Assert.assertNull(Assert.java:738)
at org.junit.Assert.assertNull(Assert.java:748)
at org.apache.hadoop.hdfs.server.namenode.TestAuditLogger.testCallerContextCharacterEscape(TestAuditLogger.java:617)
Maybe we can use separate thread here so that we can expect callerContext to be null (per ThreadLocal)?
Member
Author
There was a problem hiding this comment.
@virajjasani Thanks for your review. I fixed it to clear the caller context at the end of the unit test. ff11828
|
💔 -1 overall
This message was automatically generated. |
virajjasani
approved these changes
Nov 23, 2021
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1 (non-binding), pending QA
|
🎊 +1 overall
This message was automatically generated. |
liuml07
approved these changes
Nov 23, 2021
ferhui
approved these changes
Nov 25, 2021
Member
Author
|
Merged it. Thanks for your reviews, @virajjasani, @liuml07, @ferhui! |
omalley
pushed a commit
that referenced
this pull request
Mar 14, 2022
Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: Mingliang Liu <[email protected]> Reviewed-by: Hui Fei <[email protected]> Cherry-picked from 9c887e5 by Owen O'Malley
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…he#3698) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: Mingliang Liu <[email protected]> Reviewed-by: Hui Fei <[email protected]>
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
…he#3698) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: Mingliang Liu <[email protected]> Reviewed-by: Hui Fei <[email protected]> Cherry-picked from 9c887e5 by Owen O'Malley
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
CallerContext should not include control characters.
This PR is a different approach from #3683.
How was this patch tested?
I created a unit test and confirmed that it succeeded.
For code changes: