-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-17653. Do not use guava's Files.createTempDir(). #2945
Conversation
Change-Id: I00b0836e1f56f58ed23ffbbf6e8e9ff5409a65bf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one minor comment, else looks good overall
...main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreFileImpl.java
Show resolved
Hide resolved
Change-Id: Ifd49e036649fe9d1dd1a572b70b3ab9e01acf17a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; minor tweaks of imports suggested.
side issue: is there a way we could block curator.shaded imports? They're clearly accidental IDE ones...having them rejected in yetus &c would be great
...g/apache/hadoop/hdfs/server/common/blockaliasmap/impl/TestInMemoryLevelDBAliasMapClient.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hdfs/server/common/blockaliasmap/impl/TestLevelDbMockAliasMapClient.java
Outdated
Show resolved
Hide resolved
@steveloughran This is good one. I can give it a try in a follow-up Jira? |
We can warn the imports by updating the illegal import section in checkstyle.yml. hadoop/hadoop-build-tools/src/main/resources/checkstyle/checkstyle.xml Lines 122 to 126 in eac7aef
|
@aajisaka Although this is good option, but code can still be checked-in with this import. In addition to this change, we can also use If you are fine, I can create PR with this enforcer in parallel? |
💔 -1 overall
This message was automatically generated. |
Yes. Thank you! |
Thanks @aajisaka. I have the patch ready and done local testing to verify this as well. Will create PR soon after this current PR gets merged (until then new PR's QA bot will keep failing to build in the presence of imports). |
Change-Id: I478c4b7dbf9c31a5d4fa70e8a07a73a504c3cf17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me
💔 -1 overall
This message was automatically generated. |
trigger the precommit build again just to be on the safe side. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Do we know if any of the test failures are related or is this just highlighting all tests with issues because the patch spans the code? |
TestRouterFederationRename failure looks caused by this PR. I'll check again. |
Is it related? I applied the patch and passed the test locally. |
The test has been fixed by #2954 |
Thank you @jojochuang, @virajjasani, and @steveloughran |
Reviewed-by: Steve Loughran <stevel@apache.org> Signed-off-by: Akira Ajisaka <aajisaka@apache.org> (cherry picked from commit f1e1809)
Follow-up PR to restrict imports from org.apache.curator.shaded: #2969 |
Reviewed-by: Steve Loughran <stevel@apache.org> Signed-off-by: Akira Ajisaka <aajisaka@apache.org>
…pache#2945) Reviewed-by: Steve Loughran <stevel@apache.org> Signed-off-by: Akira Ajisaka <aajisaka@apache.org> # Conflicts: # hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/common/blockaliasmap/impl/TestInMemoryLevelDBAliasMapClient.java # hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/common/blockaliasmap/impl/TestLevelDbMockAliasMapClient.java # hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/client/ServiceClient.java # hadoop-yarn-project/hadoop-yarn/hadoop-yarn-csi/src/test/java/org/apache/hadoop/yarn/csi/client/TestCsiClient.java Change-Id: I409cb52a6c5dac7343be66ce3af0c25ba659db72
Guava's Files.createTempDir() is deprecated. The recommendation is to use JDK's java.nio.file.Files.createTempDirectory() instead.
Some of the code follows the internal implementation of Guava createTempDir() https://github.com/google/guava/blob/master/guava/src/com/google/common/io/Files.java#L423