BEAM-1567 hashStream should be closed in PackageUtil#createPackageAttributes()#2121
Closed
tedyu wants to merge 7 commits intoapache:masterfrom
tedyu:master
Closed
BEAM-1567 hashStream should be closed in PackageUtil#createPackageAttributes()#2121tedyu wants to merge 7 commits intoapache:masterfrom tedyu:master
tedyu wants to merge 7 commits intoapache:masterfrom
tedyu:master
Conversation
dhalperi
reviewed
Feb 28, 2017
| } catch (IOException e) { | ||
| throw new RuntimeException("Package setup failure for " + source, e); | ||
| } finally { | ||
| hashStream.close(); |
Contributor
There was a problem hiding this comment.
maybe close the countingOutputStream at L109?
- it's the outermost wrapper
- that will ensure the hasher has received all the bytes. (even though I happen to know that the counting stream does not buffer).
dhalperi
reviewed
Feb 28, 2017
| } catch (IOException e) { | ||
| throw new RuntimeException("Package setup failure for " + source, e); | ||
| } finally { | ||
| if (countingOutputStream != null) { |
Contributor
There was a problem hiding this comment.
Hmm. I had thought that with the closing at L109 we didn't need this, but I see that we still might.
Maybe instead just make a Closer up front and let it do all the work for you?
https://github.com/google/guava/wiki/ClosingResourcesExplained
Contributor
There was a problem hiding this comment.
(he says, hoping that CountingOutputStream is Closeable)
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 665.07 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.compiler.CompilationFailureException: Compilation failure at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1029) at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:137) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-28T01:42:23.268 [ERROR] 2017-02-28T01:42:23.268 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-28T01:42:23.268 [ERROR] 2017-02-28T01:42:23.268 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-28T01:42:23.268 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-02-28T01:42:23.268 [ERROR] 2017-02-28T01:42:23.268 [ERROR] After correcting the problems, you can resume the build with the command2017-02-28T01:42:23.268 [ERROR] mvn -rf :beam-runners-google-cloud-dataflow-javachannel stoppedSetting status of 0441370 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7928/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 665.54 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.compiler.CompilationFailureException: Compilation failure at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1029) at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:137) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-28T01:51:04.504 [ERROR] 2017-02-28T01:51:04.504 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-28T01:51:04.504 [ERROR] 2017-02-28T01:51:04.504 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-28T01:51:04.504 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-02-28T01:51:04.504 [ERROR] 2017-02-28T01:51:04.504 [ERROR] After correcting the problems, you can resume the build with the command2017-02-28T01:51:04.504 [ERROR] mvn -rf :beam-runners-google-cloud-dataflow-javachannel stoppedSetting status of 00d2c4a to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7929/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 665.02 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.compiler.CompilationFailureException: Compilation failure at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1029) at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:137) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-28T01:53:24.738 [ERROR] 2017-02-28T01:53:24.738 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-28T01:53:24.738 [ERROR] 2017-02-28T01:53:24.738 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-28T01:53:24.739 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-02-28T01:53:24.739 [ERROR] 2017-02-28T01:53:24.739 [ERROR] After correcting the problems, you can resume the build with the command2017-02-28T01:53:24.739 [ERROR] mvn -rf :beam-runners-google-cloud-dataflow-javachannel stoppedSetting status of b0f3479 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7930/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 665.01 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.compiler.CompilationFailureException: Compilation failure at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1029) at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:137) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-28T02:03:07.691 [ERROR] 2017-02-28T02:03:07.691 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-28T02:03:07.691 [ERROR] 2017-02-28T02:03:07.691 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-28T02:03:07.691 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-02-28T02:03:07.691 [ERROR] 2017-02-28T02:03:07.691 [ERROR] After correcting the problems, you can resume the build with the command2017-02-28T02:03:07.692 [ERROR] mvn -rf :beam-runners-google-cloud-dataflow-javachannel stoppedSetting status of a9ad60c to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7932/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 664.84 KB...] at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.compiler.CompilationFailureException: Compilation failure/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/PackageUtil.java:[126,19] unreported exception java.io.IOException; must be caught or declared to be thrown at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1029) at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:137) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-28T03:47:59.891 [ERROR] 2017-02-28T03:47:59.892 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-28T03:47:59.892 [ERROR] 2017-02-28T03:47:59.892 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-28T03:47:59.892 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-02-28T03:47:59.893 [ERROR] 2017-02-28T03:47:59.893 [ERROR] After correcting the problems, you can resume the build with the command2017-02-28T03:47:59.893 [ERROR] mvn -rf :beam-runners-google-cloud-dataflow-javachannel stoppedSetting status of b235984 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7941/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
Refer to this link for build results (access rights to CI server needed): |
iemejia
reviewed
Mar 1, 2017
| } finally { | ||
| try { | ||
| closer.close(); | ||
| } catch (IOException ioe) { |
Member
There was a problem hiding this comment.
All the other changes look ok, my only doubt is how to deal with this case (didn't know that Close threw IOException :S).
I will probably let Dan comment on this since this is the Dataflow runner so the ball is in google's side.
Member
|
I wonder if we could somehow validate that the closer is doing its work correctly in the test class. Apart of this + the comment on the empty catch I think is ok. Waiting for Dan's final check. |
Contributor
Author
|
Looks like Java 1.7 is required for building beam. |
|
Refer to this link for build results (access rights to CI server needed): |
Contributor
Author
|
@dhalperi Thanks |
Member
|
try/close is much better and is used all around the project, I think you can go ahead. WDYT Dan ? |
dhalperi
reviewed
Mar 2, 2017
| String stagingPath, @Nullable String overridePackageName) { | ||
| try { | ||
| Closer closer = Closer.create(); | ||
| boolean directory = source.isDirectory(); |
| Hasher hasher = Hashing.md5().newHasher(); | ||
| OutputStream hashStream = Funnels.asOutputStream(hasher); | ||
| try (CountingOutputStream countingOutputStream = new CountingOutputStream(hashStream)) { | ||
| closer.register(countingOutputStream); |
Contributor
There was a problem hiding this comment.
with these changes, which are great (thanks @iemejia !), the closer no longer does anything so we can drop it.
Contributor
There was a problem hiding this comment.
Still "need" the flush at L110, so that we guarantee the hasher has seen all bytes.
Contributor
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
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.
Here is related code:
hashStream should be closed upon return from the method