Skip to content
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-16885. Encryption zone file copy failure leaks temp file ._COP… #1859

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

xiaoyuyao
Copy link
Contributor

https://issues.apache.org/jira/browse/HADOOP-16885

Fix the leaking stream issues when access encrypted files hit exception during create.

Move the deleteOnExit to ensure the file get deleted cleanly.

private HdfsDataOutputStream safelyCreateWrappedOutputStream(
DFSOutputStream dfsos) throws IOException {
try {
return dfs.createWrappedOutputStream(dfsos, statistics);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, it looks like HBASE-16062 is related.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 23m 53s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-1 ❌ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ trunk Compile Tests _
+0 🆗 mvndep 1m 10s Maven dependency ordering for branch
+1 💚 mvninstall 21m 25s trunk passed
+1 💚 compile 18m 18s trunk passed
+1 💚 checkstyle 2m 48s trunk passed
+1 💚 mvnsite 2m 20s trunk passed
+1 💚 shadedclient 21m 31s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 1m 30s trunk passed
+0 🆗 spotbugs 2m 28s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 4m 33s trunk passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 21s Maven dependency ordering for patch
+1 💚 mvninstall 1m 36s the patch passed
+1 💚 compile 18m 29s the patch passed
+1 💚 javac 18m 29s the patch passed
+1 💚 checkstyle 3m 1s root: The patch generated 0 new + 37 unchanged - 1 fixed = 37 total (was 38)
+1 💚 mvnsite 2m 31s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 shadedclient 15m 34s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 1m 34s the patch passed
+1 💚 findbugs 5m 7s the patch passed
_ Other Tests _
-1 ❌ unit 9m 37s hadoop-common in the patch passed.
+1 💚 unit 2m 12s hadoop-hdfs-client in the patch passed.
+1 💚 asflicense 0m 46s The patch does not generate ASF License warnings.
157m 48s
Reason Tests
Failed junit tests hadoop.fs.shell.TestCopy
Subsystem Report/Notes
Docker Client=19.03.6 Server=19.03.6 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1859/1/artifact/out/Dockerfile
GITHUB PR #1859
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux cb0769523e41 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / dda00d3
Default Java 1.8.0_242
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1859/1/artifact/out/patch-unit-hadoop-common-project_hadoop-common.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1859/1/testReport/
Max. process+thread count 3233 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1859/1/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

} finally {
if (!direct) {
deleteOnExit(target.path);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I need to be confident that we're handling is the situation where the file exists at the end, create fails with an overwrite error -we don't want the deleteOnExit to suddenly delete that previous file, not if it is one we care about.

For direct writes then: we don't do that delete (good), For indirect ones, we are relying on the fact that the file being created is temporary. Because we're going to end up stamping on it aren't we?

From an S3A perspective -I don't see the codepath creating a 404 as the deleteOnExit registration takes place after the upload has succeeded: the HEAD will find the file which has just been created.

@steveloughran
Copy link
Contributor

shell-side changes LGTM. No opinion on the HDFS code

@xiaoyuyao xiaoyuyao self-assigned this Feb 27, 2020
@steveloughran steveloughran merged commit 0dd8956 into apache:trunk Mar 2, 2020
@steveloughran
Copy link
Contributor

I see @jojochuang approved, so +1

I've merged it in -thanks!

@steveloughran
Copy link
Contributor

see #1874; I hadn't noticed the hadoop-common test failure. Apologies all; if yetus is happy I'll commit it myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants