Skip to content

[BEAM-1045] IOChannelFactory: removes toPath() and resolve(), and rely on URI for paths resolving.#1412

Closed
peihe wants to merge 1 commit intoapache:masterfrom
peihe:io-channel-factory-to-path
Closed

[BEAM-1045] IOChannelFactory: removes toPath() and resolve(), and rely on URI for paths resolving.#1412
peihe wants to merge 1 commit intoapache:masterfrom
peihe:io-channel-factory-to-path

Conversation

@peihe
Copy link
Copy Markdown
Contributor

@peihe peihe commented Nov 22, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

…o tighten the interface.

@peihe peihe force-pushed the io-channel-factory-to-path branch 3 times, most recently from 98ecce8 to 771a945 Compare November 29, 2016 23:01
@peihe peihe force-pushed the io-channel-factory-to-path branch from 771a945 to 55d53f3 Compare December 2, 2016 03:16
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 2, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5446/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from 55d53f3 to 522d3ce Compare December 6, 2016 22:27
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 6, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5580/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch 3 times, most recently from 99a5de2 to f055426 Compare December 6, 2016 23:39
@peihe peihe changed the title [BEAM-951] IOChannelFactory: replace toPath() with resolveSibling() t… [BEAM-951] IOChannelFactory: removes toPath() and resolve(), and rely on URI for paths resolving. Dec 6, 2016
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 6, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5583/

Build result: FAILURE

[...truncated 10683 lines...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/spark/pom.xml to org.apache.beam/beam-runners-spark/0.4.0-incubating-SNAPSHOT/beam-runners-spark-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/io/hdfs/pom.xml to org.apache.beam/beam-sdks-java-io-hdfs/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-hdfs-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/maven-archetypes/pom.xml to org.apache.beam/beam-sdks-java-maven-archetypes-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-maven-archetypes-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/dependency-reduced-pom.xml to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-javadoc.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-sources.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-test-sources.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-test-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-tests.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/apex/pom.xml to org.apache.beam/beam-runners-apex/0.4.0-incubating-SNAPSHOT/beam-runners-apex-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/io/jms/pom.xml to org.apache.beam/beam-sdks-java-io-jms/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jms-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/flink/pom.xml to org.apache.beam/beam-runners-flink-parent/0.4.0-incubating-SNAPSHOT/beam-runners-flink-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/io/mongodb/pom.xml to org.apache.beam/beam-sdks-java-io-mongodb/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-mongodb-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/extensions/sorter/pom.xml to org.apache.beam/beam-sdks-java-extensions-sorter/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-sorter-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/pom.xml to org.apache.beam/beam-sdks-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs #5585 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/extensions/join-library/pom.xml to org.apache.beam/beam-sdks-java-extensions-join-library/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-join-library-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/pom.xml to org.apache.beam/beam-runners-parent/0.4.0-incubating-SNAPSHOT/beam-runners-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/io/jdbc/pom.xml to org.apache.beam/beam-sdks-java-io-jdbc/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jdbc-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/sdks/java/extensions/pom.xml to org.apache.beam/beam-sdks-java-extensions-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/examples/pom.xml to org.apache.beam/beam-examples-parent/0.4.0-incubating-SNAPSHOT/beam-examples-parent-0.4.0-incubating-SNAPSHOT.pomchannel stoppedSetting status of adb5283 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5583/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

@asfbot
Copy link
Copy Markdown

asfbot commented Dec 6, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5584/

Build result: FAILURE

[...truncated 10698 lines...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/hdfs/pom.xml to org.apache.beam/beam-sdks-java-io-hdfs/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-hdfs-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/maven-archetypes/pom.xml to org.apache.beam/beam-sdks-java-maven-archetypes-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-maven-archetypes-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/dependency-reduced-pom.xml to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-javadoc.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-sources.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-test-sources.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-test-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-tests.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/apex/pom.xml to org.apache.beam/beam-runners-apex/0.4.0-incubating-SNAPSHOT/beam-runners-apex-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/jms/pom.xml to org.apache.beam/beam-sdks-java-io-jms/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jms-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/flink/pom.xml to org.apache.beam/beam-runners-flink-parent/0.4.0-incubating-SNAPSHOT/beam-runners-flink-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/mongodb/pom.xml to org.apache.beam/beam-sdks-java-io-mongodb/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-mongodb-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/sorter/pom.xml to org.apache.beam/beam-sdks-java-extensions-sorter/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-sorter-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/pom.xml to org.apache.beam/beam-sdks-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs #5585 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/join-library/pom.xml to org.apache.beam/beam-sdks-java-extensions-join-library/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-join-library-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/pom.xml to org.apache.beam/beam-runners-parent/0.4.0-incubating-SNAPSHOT/beam-runners-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: Runners #5585 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/jdbc/pom.xml to org.apache.beam/beam-sdks-java-io-jdbc/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jdbc-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/pom.xml to org.apache.beam/beam-sdks-java-extensions-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/examples/pom.xml to org.apache.beam/beam-examples-parent/0.4.0-incubating-SNAPSHOT/beam-examples-parent-0.4.0-incubating-SNAPSHOT.pomchannel stoppedSetting status of 99a5de2 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5584/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

@asfbot
Copy link
Copy Markdown

asfbot commented Dec 6, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5585/

Build result: FAILURE

[...truncated 10693 lines...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/maven-archetypes/examples/pom.xml to org.apache.beam/beam-sdks-java-maven-archetypes-examples/0.4.0-incubating-SNAPSHOT/beam-sdks-java-maven-archetypes-examples-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/spark/pom.xml to org.apache.beam/beam-runners-spark/0.4.0-incubating-SNAPSHOT/beam-runners-spark-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/hdfs/pom.xml to org.apache.beam/beam-sdks-java-io-hdfs/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-hdfs-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/maven-archetypes/pom.xml to org.apache.beam/beam-sdks-java-maven-archetypes-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-maven-archetypes-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/dependency-reduced-pom.xml to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-javadoc.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-sources.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-test-sources.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-test-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/google-cloud-dataflow-java/target/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-tests.jar to org.apache.beam/beam-runners-google-cloud-dataflow-java/0.4.0-incubating-SNAPSHOT/beam-runners-google-cloud-dataflow-java-0.4.0-incubating-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/apex/pom.xml to org.apache.beam/beam-runners-apex/0.4.0-incubating-SNAPSHOT/beam-runners-apex-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/jms/pom.xml to org.apache.beam/beam-sdks-java-io-jms/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jms-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/flink/pom.xml to org.apache.beam/beam-runners-flink-parent/0.4.0-incubating-SNAPSHOT/beam-runners-flink-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/mongodb/pom.xml to org.apache.beam/beam-sdks-java-io-mongodb/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-mongodb-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/sorter/pom.xml to org.apache.beam/beam-sdks-java-extensions-sorter/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-sorter-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/pom.xml to org.apache.beam/beam-sdks-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/join-library/pom.xml to org.apache.beam/beam-sdks-java-extensions-join-library/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-join-library-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/pom.xml to org.apache.beam/beam-runners-parent/0.4.0-incubating-SNAPSHOT/beam-runners-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/jdbc/pom.xml to org.apache.beam/beam-sdks-java-io-jdbc/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jdbc-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/pom.xml to org.apache.beam/beam-sdks-java-extensions-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-parent-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/examples/pom.xml to org.apache.beam/beam-examples-parent/0.4.0-incubating-SNAPSHOT/beam-examples-parent-0.4.0-incubating-SNAPSHOT.pomchannel stoppedSetting status of f055426 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5585/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from f055426 to 3608afb Compare December 7, 2016 01:38
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 7, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5592/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from 3608afb to 146b3a6 Compare December 7, 2016 02:11
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 7, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5594/

Build result: FAILURE

[...truncated 6597 lines...]No artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: Runners :: Google Cloud Dataflow #5595 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/apex/pom.xml to org.apache.beam/beam-runners-apex/0.4.0-incubating-SNAPSHOT/beam-runners-apex-0.4.0-incubating-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/jms/pom.xml to org.apache.beam/beam-sdks-java-io-jms/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jms-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs :: Java :: IO :: JMS #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/flink/pom.xml to org.apache.beam/beam-runners-flink-parent/0.4.0-incubating-SNAPSHOT/beam-runners-flink-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: Runners :: Flink #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/mongodb/pom.xml to org.apache.beam/beam-sdks-java-io-mongodb/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-mongodb-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs :: Java :: IO :: MongoDB #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/sorter/pom.xml to org.apache.beam/beam-sdks-java-extensions-sorter/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-sorter-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs :: Java :: Extensions :: Sorter #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/pom.xml to org.apache.beam/beam-sdks-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs #5595 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/join-library/pom.xml to org.apache.beam/beam-sdks-java-extensions-join-library/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-join-library-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs :: Java :: Extensions :: Join library #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/runners/pom.xml to org.apache.beam/beam-runners-parent/0.4.0-incubating-SNAPSHOT/beam-runners-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: Runners #5595 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/io/jdbc/pom.xml to org.apache.beam/beam-sdks-java-io-jdbc/0.4.0-incubating-SNAPSHOT/beam-sdks-java-io-jdbc-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs :: Java :: IO :: JDBC #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/java/extensions/pom.xml to org.apache.beam/beam-sdks-java-extensions-parent/0.4.0-incubating-SNAPSHOT/beam-sdks-java-extensions-parent-0.4.0-incubating-SNAPSHOT.pomNo artifacts from beam_PreCommit_Java_MavenInstall » Apache Beam :: SDKs :: Java :: Extensions #5593 to compare, so performing full copy of artifacts[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/examples/pom.xml to org.apache.beam/beam-examples-parent/0.4.0-incubating-SNAPSHOT/beam-examples-parent-0.4.0-incubating-SNAPSHOT.pomchannel stoppedSetting status of 146b3a6 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5594/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from 146b3a6 to 00c1353 Compare December 7, 2016 02:46
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 7, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5597/
--none--

@peihe
Copy link
Copy Markdown
Contributor Author

peihe commented Dec 7, 2016

R: @jkff @dhalperi

@peihe peihe force-pushed the io-channel-factory-to-path branch from 00c1353 to c76c777 Compare December 7, 2016 02:54
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 7, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5598/
--none--

Copy link
Copy Markdown
Contributor

@jkff jkff left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the delay!

* absolute paths, then this method returns a path that starts with the last absolute path
* in {@code others} joined with the remaining paths.
*/
public static String resolveAgainstDirectory(String directory, String... others) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the decision to make this class operate on String rather than URI?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This function is to accommodate URI and Path resolves differently.
If the callers already have URIs, their uris should be constructed follow the URI spec (having "/" at the end for directories). Then, they can just use URI.resolve().

URI.create("/home/a").resolve("b").resolve("c").toString() is "/home/c". (Path will give "/home/a/b/c")
URI.create("/home/a/").resolve("b/").resolve("c").toString() is "/home/a/b/c".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I see. However, it seems like an easy-to-misuse API: the new FileSystems API will be phrased in terms of URIs, and people will need to remember that despite that, URI.resolve() does the wrong thing, and they should use PathUtils.resolve() on uri.toString() instead. Is there any way to address this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see other way.

I wouldn't say URI.resolve() does the wrong thing. People who uses URI.resolve() directly should understand that API. And, unit tests will catch the unexpected behavior easily.
This requirement is for library writers, and I think it is reasonable.

* absolute paths, then this method returns a path that starts with the last absolute path
* in {@code others} joined with the remaining paths.
*/
public static String resolveAgainstDirectory(String directory, String... others) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you need "others" to be an array rather than a single element?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am following the previous pattern in IOChannelUtils.

Also, I think the client code is more readable when resolving multiple levels directories.

}

/**
* Returns the directory portion of the {@code path}, which includes all but the file name.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Document what does this return if the path ends with a / ?
What does this return if this is a path something like "gs://bucket"?
What does this return for ".." or "." or "~" ? Are these even allowed?
etc.
In general, examples would be very helpful in docs to the methods of this whole class. There was a lot of ambiguity in docs of IOChannelUtils and since we're revamping the whole thing it'd be great to also do comments properly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the javadoc, and simplified the implementation with URI.

* or an empty {@code String} if this path is "/" or empty,
* or {@code null} if this path is {@code null}.
*/
public static String getFileName(String path) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same questions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated


// Windows doesn't like resolving paths with * in them, so the * is appended after resolve.
assertThat(factory.match(factory.resolve(temporaryFolder.getRoot().getPath(), "b") + "*"),
// Windows doesn't like resolving paths with * in them,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this still true for the new implementation? (also in other places in this file)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

/**
* Unit tests for {@link PathUtils}.
*/
public class PathUtilsTest {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All tests here are for a local-file path. Add tests for GCS- or S3-like paths, with buckets and stuff?

Also, I think the path resolution functions should work on non-existing files too, so you don't need an actual TemporaryFolder, you can hard-code the path of a fictional folder ("/tmp/foo" for example). The tests will probably get much cleaner since you'll drop all those tmpFolder.getRoot().toPath() calls and so on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add more non-trivial examples - specifically for the cases where it would be non-trivial for a human to figure out what should be the right answer. Handling the common cases is easy; a big part of the job of a class for working with filenames is defining how the corner cases are handled, in a useful way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

}

@Test
public void testResolveOtherIsFullPath() throws Exception {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By "Full" you mean "Absolute"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

@asfbot
Copy link
Copy Markdown

asfbot commented Dec 9, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5688/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from 2040311 to f29cc78 Compare December 9, 2016 07:14
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 9, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5692/
--none--

@peihe
Copy link
Copy Markdown
Contributor Author

peihe commented Dec 9, 2016

PTAL

private static final String URI_DELIMITER = "/";

/**
* Resolve multiple {@code others} against the {@code directory} sequentially.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Phrasing cleanup:

Resolves multiple path fragments against the given directory sequentially.

Unlike URI#resolve, this function appends relative path fragments rather than replaces them, e.g. (give example).

* absolute paths, then this method returns a path that starts with the last absolute path
* in {@code others} joined with the remaining paths.
*/
public static String resolveAgainstDirectory(String directory, String... others) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I see. However, it seems like an easy-to-misuse API: the new FileSystems API will be phrased in terms of URIs, and people will need to remember that despite that, URI.resolve() does the wrong thing, and they should use PathUtils.resolve() on uri.toString() instead. Is there any way to address this?

* <p>Unlike {@link URI#resolve}, {@link #resolveAgainstDirectory} includes the last segment of
* the path.
*
* <p>Empty paths in {@code others} are ignored. If {@code others} contains one or more
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why allow empty paths at all?

}

private static String resolveAgainstDirectory(String directory, String other) {
if (Strings.isNullOrEmpty(other)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why allow nulls?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added checkNotNull

*
* @return a string representing the name of the file or directory,
* or an empty {@code String} if this path is "/" or empty,
* or {@code null} if this path is {@code null}.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why allow nulls?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated javadoc

*
* @return a string representing the directory portion of the {@code path},
* or the {@code path} itself, if it is already a directory,
* or an empty {@link String}, if {@code path} is "~", ".", "..".
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did not mean to ask for specifically documenting ~, . and .. - I would like to see the documentation describe more concretely the general rule by which it operates in all such ambiguous cases, because these are not the only ambiguous cases. And illustrate the rule by examples.

E.g. what will it return for "gs://bucket"? For "gs://"? For "file:///" ? For "/foo//bar"? For "/foo/bar/../" ? For "/foo/bar/.."? For other cases I haven't thought of?

* the root in the directory hierarchy.
*
* @return a string representing the name of the file or directory,
* or the {@code path} itself, if {@code path} is "~", ".", "..",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same questions as to the previous method (ambiguities, disallowing nulls etc)

* @return a string representing the name of the file or directory,
* or the {@code path} itself, if {@code path} is "~", ".", "..",
* or an empty {@link String} if this path is "/" or empty,
* or {@code null} if this path is {@code null}.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment about nulls is incorrect, the code throws on null.

checkNotNull(path, "path");
if (path.isEmpty() || path.equals(URI_DELIMITER)) {
return "";
} else if (path.endsWith(URI_DELIMITER)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I really don't like this hand-crafted string manipulation - it looks naive, in the sense that there are probably corner cases it doesn't cover. Are there any utilities we can reuse?

/**
* Unit tests for {@link PathUtils}.
*/
public class PathUtilsTest {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add more non-trivial examples - specifically for the cases where it would be non-trivial for a human to figure out what should be the right answer. Handling the common cases is easy; a big part of the job of a class for working with filenames is defining how the corner cases are handled, in a useful way.

@peihe peihe force-pushed the io-channel-factory-to-path branch from f29cc78 to 4a9ca48 Compare December 14, 2016 06:52
@peihe peihe force-pushed the io-channel-factory-to-path branch 2 times, most recently from 9d1d1e2 to 8daee59 Compare December 20, 2016 19:20
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 20, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6123/
--none--

@asfbot
Copy link
Copy Markdown

asfbot commented Dec 20, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6120/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from 8daee59 to 002a130 Compare December 22, 2016 22:11
@asfbot
Copy link
Copy Markdown

asfbot commented Dec 22, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6230/
--none--

} catch (IOException e) {
throw new RuntimeException(e);
}
URI baseOutputUri = URI.create(baseOutputFilename).normalize();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • This fragment looks like something that should a single call to PathUtils - AFAIU the whole point of PathUtils is to free clients from this error-prone string and URI manipulation.
  • I don't understand what's going on here in general:
    ** does URI.create() work for Windows paths (e.g. "c:\foo bar\blah")?
    ** What's the difference between baseOutputUri and pathUri?
    ** What does the self-relativize in "pathUri.resolve("").relativize(pathUri)" accomplish?
    ** Why do we hand-concatenate paths at line 332 instead of using .resolve()?
  • Line 323 says we shouldn't call Instant.now() inside .apply(), but at line 334 we're calling it - I'm surprised there's no test for that, can you add one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My worries about adding this logic to PathUtils is that people might misuse it.
I think PathUtils should only contain common URI manipulation. And, they need to be simple and easy to understand to avoid being misused.
The logic here is not simple, and the client should own it: understand the URI manipulation, and verify the code with sufficient tests.

  • non-URI paths needs to be converted before reaching to here.
  • pathUri doesn't contains scheme, authority, query and fragment.
  • self-relativize is to get the last component of pathUri.
    "/home/output" resolve("") returns "/home/"
    "/home/" relativize("/home/output") returns "output"
  • line 332 is constructing the directory name. It is not path resolving (append /).

3.added test for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 - ok, I'm convinced.

  1. "non-URI paths need to be converted" - which code does that? In other words: does the current PR change user-facing behavior of FileBasedSink in a backward-incompatible way on Windows, i.e. will it break existing user pipelines? If yes, please fix that; if no, please explain why not, because if I'm understanding correctly the answer currently is yes.

On bullets 2,3 - please put them in comments in code - I asked about them because they are unclear from the code; and ideally they should be.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

* @throws IllegalArgumentException if others contains {@link URI} query or fragment components.
*/
public static String resolveAgainstDirectory(
@Nonnull String directory, @Nonnull String other, @Nonnull String... others) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Annotating as Nonnull is unnecessary - everything is non-null unless annotated as Nullable. This is the style followed by the rest of Beam codebase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


String path = directory.getPath();
if (!Strings.isNullOrEmpty(directory.getAuthority()) && Strings.isNullOrEmpty(path)) {
// Workaround of [BEAM-1174]: path needs to be absolute if the authority exists.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

.apply("/home/output/"),
Matchers.startsWith("/home/output/temp-beam--"));

// Tests for Windows OS path in URI format.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about Windows OS path in non-URI format: it's currently allowed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Currently, Windows OS paths in non-URI format don't work. It breaks when our code resolves *.

I would like FileBasedSource/Sink to use URI, and TextIO/AvroIO own the conversion from users string to URI.

For users' convenience, we can try converting Windows OS path to the URI format in TextIO/AvroIO for some common cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By "currently" do you mean "at Beam HEAD", or "at the current PR"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I mean "at Beam HEAD", see https://issues.apache.org/jira/browse/BEAM-1045

This PR removes toPath() and resolve(). It fixed most of BEAM-1045. (still need to fix match())
I updated the PR title to refer to BEAM-1045

public void testBuildTemporaryDirectoryName() {
// Tests for local files without the scheme.
assertThat(
new FileBasedWriteOperation.TemporaryDirectoryBuilder()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extract new FileBasedWriteOperation.TemporaryDirectoryBuilder() into a local variable and reformat?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

assertThat(
new FileBasedWriteOperation.TemporaryDirectoryBuilder()
.apply("/home/output/"),
Matchers.startsWith("/home/output/temp-beam--"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be good to special-case this case where there is no base filename, to avoid producing the confusing -- - it looks like a bug even though it isn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

// Tests absolute path.
assertEquals(
"/root/tmp/aa",
PathUtils.resolveAgainstDirectory("/root/tmp/aa", "/root/tmp/aa"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is the original and resolved path the same?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated to different strings

// Tests query and fragment.
assertEquals(
"file:/home/output/aa/bb",
PathUtils.resolveAgainstDirectory("file:/home/output?query#fragment", "aa", "bb"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like we're always dropping query and fragment. Why even allow them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resolve() in Path or URI do the same thing.

Also, I don't see reasons to disable query and fragment in TextIO.Write.to().
If we don't allow them in resolveAgainstDirectory(), then we have to explicitly drop them every time before invoking this method. It is not convenient, and could lead to unnecessary check fails.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reason to disallow query and fragment is that we're dropping them - if the user specifies them, they probably expect that we'll preserve this data, but we won't - dropping data should not be done silently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done
checked in resolveAgainstDirectory()

@peihe
Copy link
Copy Markdown
Contributor Author

peihe commented Jan 9, 2017

PTAL

@asfbot
Copy link
Copy Markdown

asfbot commented Jan 9, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6473/
--none--

@peihe peihe changed the title [BEAM-951] IOChannelFactory: removes toPath() and resolve(), and rely on URI for paths resolving. [BEAM-1045] IOChannelFactory: removes toPath() and resolve(), and rely on URI for paths resolving. Jan 11, 2017
@asfbot
Copy link
Copy Markdown

asfbot commented Jan 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6522/
--none--

@jkff
Copy link
Copy Markdown
Contributor

jkff commented Jan 14, 2017

Is this ready for another round?... Please get Jenkins green.

@asfbot
Copy link
Copy Markdown

asfbot commented Jan 14, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6556/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from d31d588 to b65d5b2 Compare January 17, 2017 23:34
@asfbot
Copy link
Copy Markdown

asfbot commented Jan 17, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6600/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from b65d5b2 to 2161f7d Compare January 17, 2017 23:51
@asfbot
Copy link
Copy Markdown

asfbot commented Jan 17, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6602/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch 2 times, most recently from cd59258 to f11f9f8 Compare January 18, 2017 00:25
@peihe
Copy link
Copy Markdown
Contributor Author

peihe commented Jan 18, 2017

Updated the PR, waiting for tests.
I did:
Rebased PR to resolve conflicts
Addressed feedback in commit ffe0238
Merged PathUtils into FileSystems in commit f11f9f8

@asfbot
Copy link
Copy Markdown

asfbot commented Jan 18, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6604/
--none--

@asfbot
Copy link
Copy Markdown

asfbot commented Jan 18, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6607/
--none--

@peihe
Copy link
Copy Markdown
Contributor Author

peihe commented Jan 18, 2017

I hit a issue of URI in my other PR.
Let's me re-think about it.

I will ping when it is ready for review.

@peihe peihe force-pushed the io-channel-factory-to-path branch from f11f9f8 to 6bef8dd Compare January 26, 2017 00:03
@asfbot
Copy link
Copy Markdown

asfbot commented Jan 26, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6815/
--none--

@peihe peihe force-pushed the io-channel-factory-to-path branch from 6bef8dd to b7517bf Compare January 26, 2017 19:57
@asfbot
Copy link
Copy Markdown

asfbot commented Jan 26, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6833/
--none--

@peihe
Copy link
Copy Markdown
Contributor Author

peihe commented Feb 14, 2017

Close this PR, and resolve() is done in #1900

Thanks for Eugene for reviewing. Many of the feedback are adopted in the PR above.

@peihe peihe closed this Feb 14, 2017
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.

3 participants