[BEAM-59] Beam FileSystem: ResourceId and its local and gcs implementation. #1900
Conversation
|
R: @dhalperi @davorbonaci |
peihe
force-pushed
the
file-system-local-gs-resolve
branch
2 times, most recently
from
bb907f3
to
d26796e
Compare
peihe
changed the title
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
peihe
force-pushed
the
file-system-local-gs-resolve
branch
from
d347dfc
to
f611af5
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 5261 lines...] at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:77) at org.codehaus.groovy.reflection.CachedConstructor.doConstructorInvoke(CachedConstructor.java:71) at org.codehaus.groovy.runtime.callsite.ConstructorSite$ConstructorSiteNoUnwrap.callConstructor(ConstructorSite.java:81) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallConstructor(CallSiteArray.java:57) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:230) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:242) at org.codehaus.mojo.findbugs.FindbugsViolationCheckMojo.execute(FindbugsViolationCheckMojo.groovy:530) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-02T21:44:31.889 [ERROR] 2017-02-02T21:44:31.889 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-02T21:44:31.889 [ERROR] 2017-02-02T21:44:31.889 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-02T21:44:31.889 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-02T21:44:31.889 [ERROR] 2017-02-02T21:44:31.889 [ERROR] After correcting the problems, you can resume the build with the command2017-02-02T21:44:31.889 [ERROR] mvn -rf :beam-sdks-java-io-google-cloud-platformchannel stoppedSetting status of ec9b681 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7027/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
peihe
force-pushed
the
file-system-local-gs-resolve
branch
2 times, most recently
from
a7fd2de
to
57a9463
Compare
|
PTAL Verified locally that followings passed: |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): |
sdks/java/core/pom.xml
Outdated
| @@ -372,6 +372,12 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>org.apache.commons</groupId> | |||
| <artifactId>commons-lang3</artifactId> | |||
| <version>3.5</version> | |||
There was a problem hiding this comment.
to the extent possible, no version numbers here. Move to dependencyManagement in root pom.
| || resolveOptions.equals(StandardResolveOptions.RESOLVE_DIRECTORY), | ||
| String.format("ResolveOptions: [%s] is not supported.", resolveOptions)); | ||
| if (resolveOptions.equals(StandardResolveOptions.RESOLVE_FILE)) { | ||
| checkArgument( |
There was a problem hiding this comment.
does this need to better check whether the caller has escaped the trailing /?
There was a problem hiding this comment.
I think callers should not escape chars in "other", given escaping could be different in different file systems.
(see the comment above for whether file systems should escape or throw for unsupported chars.)
| assertNotEquals( | ||
| toResourceIdentifier("gs://my_bucket/tmp"), | ||
| toResourceIdentifier("gs://my_bucket/tmp/")); | ||
| } |
There was a problem hiding this comment.
test that toResourceIdentifier("gs://bucket") is a directory?
There was a problem hiding this comment.
test that toResourceIdentifier("gs://") is invalid?
There was a problem hiding this comment.
test that toResourceIdentifier("gs://bucket") is a directory?
I don't see this one.
There was a problem hiding this comment.
I think isDirectory() is the implementation details.
I added more tests to make sure public methods work correctly.
| @@ -440,7 +440,7 @@ public Path resolveSibling(Path other) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Path resolveSibling(String other) { | |||
| public GcsPath resolveSibling(String other) { | |||
There was a problem hiding this comment.
related to this PR in any way? Can't see why.
| } | ||
|
|
||
| @Override | ||
| public ResourceId resolve(String other, ResolveOptions resolveOptions) { |
There was a problem hiding this comment.
can you remind me why we don't do any escaping of other here? e.g., into valid URI format?
There was a problem hiding this comment.
Added more details in javadoc.
We already have GcsPath, so we don't need to use URI to store the path.
I think GcsPath only take legal names and valid chars for gcs, and don't need to escape them.
If other contains any chars that is illegal in gcs, I think the current behavior (throwing) is right. Otherwise, the pipeline might read different files or write to different places.
| @@ -440,7 +440,7 @@ public Path resolveSibling(Path other) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Path resolveSibling(String other) { | |||
| public GcsPath resolveSibling(String other) { | |||
sdks/java/core/pom.xml
Outdated
| @@ -372,6 +372,12 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>org.apache.commons</groupId> | |||
| <artifactId>commons-lang3</artifactId> | |||
| <version>3.5</version> | |||
| assertNotEquals( | ||
| toResourceIdentifier("gs://my_bucket/tmp"), | ||
| toResourceIdentifier("gs://my_bucket/tmp/")); | ||
| } |
| || resolveOptions.equals(StandardResolveOptions.RESOLVE_DIRECTORY), | ||
| String.format("ResolveOptions: [%s] is not supported.", resolveOptions)); | ||
| if (resolveOptions.equals(StandardResolveOptions.RESOLVE_FILE)) { | ||
| checkArgument( |
There was a problem hiding this comment.
I think callers should not escape chars in "other", given escaping could be different in different file systems.
(see the comment above for whether file systems should escape or throw for unsupported chars.)
| } | ||
|
|
||
| @Override | ||
| public ResourceId resolve(String other, ResolveOptions resolveOptions) { |
There was a problem hiding this comment.
Added more details in javadoc.
We already have GcsPath, so we don't need to use URI to store the path.
I think GcsPath only take legal names and valid chars for gcs, and don't need to escape them.
If other contains any chars that is illegal in gcs, I think the current behavior (throwing) is right. Otherwise, the pipeline might read different files or write to different places.
|
@dhalperi We can have HadoopResourceId that built on top of hadoop path, and UriResourceId that built on top of URI. And, we don't want to expose them as well. |
|
also, added tests for GetScheme(). |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 12287 lines...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:332) 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.MojoExecutionException: Dependency problems found at org.apache.maven.plugin.dependency.analyze.AbstractAnalyzeMojo.execute(AbstractAnalyzeMojo.java:260) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-04T00:08:03.876 [ERROR] 2017-02-04T00:08:03.877 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-04T00:08:03.877 [ERROR] 2017-02-04T00:08:03.877 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-04T00:08:03.877 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-04T00:08:03.877 [ERROR] 2017-02-04T00:08:03.877 [ERROR] After correcting the problems, you can resume the build with the command2017-02-04T00:08:03.877 [ERROR] mvn -rf :beam-runners-apexchannel stoppedSetting status of 3d6952e to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7071/ 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 11958 lines...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:332) 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.MojoExecutionException: Dependency problems found at org.apache.maven.plugin.dependency.analyze.AbstractAnalyzeMojo.execute(AbstractAnalyzeMojo.java:260) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-04T00:53:11.763 [ERROR] 2017-02-04T00:53:11.763 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-04T00:53:11.763 [ERROR] 2017-02-04T00:53:11.763 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-04T00:53:11.763 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-04T00:53:11.763 [ERROR] 2017-02-04T00:53:11.763 [ERROR] After correcting the problems, you can resume the build with the command2017-02-04T00:53:11.763 [ERROR] mvn -rf :beam-runners-apexchannel stoppedSetting status of 888400f to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7075/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 11960 lines...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:332) 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.MojoExecutionException: Dependency problems found at org.apache.maven.plugin.dependency.analyze.AbstractAnalyzeMojo.execute(AbstractAnalyzeMojo.java:260) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-06T09:39:28.604 [ERROR] 2017-02-06T09:39:28.604 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-06T09:39:28.604 [ERROR] 2017-02-06T09:39:28.604 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-06T09:39:28.604 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-06T09:39:28.604 [ERROR] 2017-02-06T09:39:28.604 [ERROR] After correcting the problems, you can resume the build with the command2017-02-06T09:39:28.604 [ERROR] mvn -rf :beam-runners-apexchannel stoppedSetting status of 888400f to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7106/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
Refer to this link for build results (access rights to CI server needed): |
|
PTAL Tests looks fine to me: Jenkins passed, travis half passed, the other exceeded the time limit. |
| Path parent = path.getParent(); | ||
| checkState( | ||
| parent != null, | ||
| String.format("Path: [%s] doesn't have the current directory.", path)); |
There was a problem hiding this comment.
I don't quite understand this case.
There was a problem hiding this comment.
This is when it is a file, but don't have the parent directory. such as the case in testGetCurrentDirectoryInvalid().
There was a problem hiding this comment.
updated this and tests.
| /** | ||
| * An identifier which represents a resource. | ||
| * | ||
| * <p>{@code ResourceId} is hierarchical and composed of a sequence of directory |
| import org.apache.beam.sdk.io.fs.ResolveOptions.StandardResolveOptions; | ||
|
|
||
| /** | ||
| * An identifier which represents a resource. |
There was a problem hiding this comment.
is this literally any resource? or is it a file-like resource?
There was a problem hiding this comment.
updated
I was trying not to using word "file" to avoid confusing, because some people think directory is not file. But, maybe it is okay.
| public interface ResourceId { | ||
|
|
||
| /** | ||
| * Resolves the given {@code String} against this {@code ResourceId}. |
There was a problem hiding this comment.
This comment is circularly-defined: what does "resolve" mean?
| * <p>For example: | ||
| * | ||
| * <pre>{@code | ||
| * ResourceId homeDir = ... |
There was a problem hiding this comment.
what is in the ... here. How is a caller expected to ever get an instance of ResourceId?
There was a problem hiding this comment.
... is used in other javadoc as well such as PTransform to represent some code that constructs the type.
I added TODO in the ResourceID javadoc about how it is constructed.
| * Get the scheme which defines the namespace of the {@link ResourceId}. | ||
| * | ||
| * <p>The scheme is required to follow URI scheme syntax. | ||
| * @see <a href="https://www.ietf.org/rfc/rfc2396.txt">RFC 2396</a> |
There was a problem hiding this comment.
newline before @see, or if you want it part of this paragraph then don't use the Javadoc @see tag
There was a problem hiding this comment.
updated, I want it part of the paragraph.
| * returns a {@code ResourceId} which matches all directories in this {@code ResourceId}. | ||
| * </ul> | ||
| * | ||
| * @throws IllegalArgumentException if other contains illegal characters or is an illegal name. |
| * </ul> | ||
| * | ||
| * @throws IllegalArgumentException if other contains illegal characters or is an illegal name. | ||
| * It is recommended that callers use common characters, such as [_a-zA-Z0-9-], in {@code other}. |
There was a problem hiding this comment.
and is - legal here? Should it be escaped?
| */ | ||
|
|
||
| /** | ||
| * Beam FileSystem interfaces and their default implementations. |
| public void testResolve() throws Exception { | ||
| if (SystemUtils.IS_OS_WINDOWS) { | ||
| // Skip tests | ||
| return; |
There was a problem hiding this comment.
We need to run tests on windows. If there are dual tests below that only run on Windows, then name this something like testResolveUnix
| import org.apache.beam.sdk.io.fs.ResolveOptions.StandardResolveOptions; | ||
|
|
||
| /** | ||
| * An identifier which represents a resource. |
There was a problem hiding this comment.
updated
I was trying not to using word "file" to avoid confusing, because some people think directory is not file. But, maybe it is okay.
| /** | ||
| * An identifier which represents a resource. | ||
| * | ||
| * <p>{@code ResourceId} is hierarchical and composed of a sequence of directory |
| * | ||
| * <p>{@code ResourceId} is hierarchical and composed of a sequence of directory | ||
| * and file name elements separated by a special separator or delimiter. | ||
| */ |
There was a problem hiding this comment.
added TODO
we need match() to construct ReousrceId.
| public interface ResourceId { | ||
|
|
||
| /** | ||
| * Resolves the given {@code String} against this {@code ResourceId}. |
| * <p>For example: | ||
| * | ||
| * <pre>{@code | ||
| * ResourceId homeDir = ... |
There was a problem hiding this comment.
... is used in other javadoc as well such as PTransform to represent some code that constructs the type.
I added TODO in the ResourceID javadoc about how it is constructed.
| * Get the scheme which defines the namespace of the {@link ResourceId}. | ||
| * | ||
| * <p>The scheme is required to follow URI scheme syntax. | ||
| * @see <a href="https://www.ietf.org/rfc/rfc2396.txt">RFC 2396</a> |
There was a problem hiding this comment.
updated, I want it part of the paragraph.
| */ | ||
|
|
||
| /** | ||
| * Beam FileSystem interfaces and their default implementations. |
| public void testResolve() throws Exception { | ||
| if (SystemUtils.IS_OS_WINDOWS) { | ||
| // Skip tests | ||
| return; |
| Path parent = path.getParent(); | ||
| checkState( | ||
| parent != null, | ||
| String.format("Path: [%s] doesn't have the current directory.", path)); |
There was a problem hiding this comment.
updated this and tests.
| * </ul> | ||
| * | ||
| * @throws IllegalArgumentException if other contains illegal characters or is an illegal name. | ||
| * It is recommended that callers use common characters, such as [_a-zA-Z0-9-], in {@code other}. |
|
LGTM ; please self-merge when green. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 10136 lines...] ^/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/harness/src/main/java/org/apache/beam/fn/harness/stream/StreamObserverFactory.java:31: error: reference not found * Uses {@link PipelineOptions} to configure which underlying {@link StreamObserver} implementation ^Command line was: /usr/local/asfpackages/java/jdk1.8.0_102/jre/../bin/javadoc @options @packagesRefer to the generated Javadoc files in '/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/harness/target/apidocs' dir. at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeJavadocCommandLine(AbstractJavadocMojo.java:5163) at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeReport(AbstractJavadocMojo.java:2075) at org.apache.maven.plugin.javadoc.JavadocJar.execute(JavadocJar.java:188) ... 33 more2017-02-08T00:57:02.858 [ERROR] 2017-02-08T00:57:02.858 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-08T00:57:02.859 [ERROR] 2017-02-08T00:57:02.859 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-08T00:57:02.859 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-08T00:57:02.859 [ERROR] 2017-02-08T00:57:02.859 [ERROR] After correcting the problems, you can resume the build with the command2017-02-08T00:57:02.859 [ERROR] mvn -rf :beam-sdks-java-harnesschannel stoppedSetting status of 32cb6ef to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7175/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): Build result: ABORTED[...truncated 8362 lines...] ... 31 moreCaused by: java.lang.RuntimeException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?Command was /bin/sh -c cd /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/harness && /usr/local/asfpackages/java/jdk1.8.0_102/jre/bin/java org.apache.maven.surefire.booter.ForkedBooter /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/harness/target/surefire/surefire6713076340287803542tmp /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/harness/target/surefire/surefire_168427999639295037100tmp at org.apache.maven.plugin.surefire.booterclient.ForkStarter.fork(ForkStarter.java:590) at org.apache.maven.plugin.surefire.booterclient.ForkStarter.fork(ForkStarter.java:460) at org.apache.maven.plugin.surefire.booterclient.ForkStarter.run(ForkStarter.java:229) at org.apache.maven.plugin.surefire.booterclient.ForkStarter.run(ForkStarter.java:201) at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeProvider(AbstractSurefireMojo.java:1026) at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked(AbstractSurefireMojo.java:862) at org.apache.maven.plugin.surefire.AbstractSurefireMojo.execute(AbstractSurefireMojo.java:755) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) ... 32 more2017-02-08T04:24:45.926 [ERROR] 2017-02-08T04:24:45.926 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-08T04:24:45.926 [ERROR] 2017-02-08T04:24:45.926 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-08T04:24:45.926 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException2017-02-08T04:24:45.926 [ERROR] 2017-02-08T04:24:45.926 [ERROR] After correcting the problems, you can resume the build with the command2017-02-08T04:24:45.926 [ERROR] mvn -rf :beam-sdks-java-harnessBuild was abortedchannel stoppedSetting status of 32cb6ef to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7183/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
peihe
force-pushed
the
file-system-local-gs-resolve
branch
from
32cb6ef
to
4cbb167
Compare
|
Changes Unknown when pulling 4cbb167 on peihe:file-system-local-gs-resolve into ** on apache:master**. |
|
Refer to this link for build results (access rights to CI server needed): |
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.