[BEAM-2277] Add ResourceIdTester and test existing ResourceId implementations #3121
Conversation
This file contains 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
Contributor
dhalperi
commented
May 12, 2017
•
dhalperi
commented
- Mark everything FileSystem-related as Experimental.
- Add ResourceIdTester, which validates a ResourceId implementation.
- Update HadoopResourceId to meet spec.
- Rename FileSystems#setDefaultConfigInWorkers and document.
- Further improve Scheme determining and rename variable away from URI.
dhalperi
force-pushed
the
b2277-resourceid-tester
branch
from
e258d55
to
1420d7f
Compare
lukecwik
requested changes
May 12, 2017
| file2.getCurrentDirectory().toString()) | ||
| .testEquals(); | ||
|
|
||
| // TODO: test resolving strings that need to be escaped (what is the spec?). |
There was a problem hiding this comment.
The URI spec has a set of delimiter characters, the issue is that %41 and A are the same from the URI perspective and should really go through a URI normalizer if we rely on the string versions for anything.
Add this link to your comments which talks about the characters specifically:
https://tools.ietf.org/html/rfc3986#section-2
|
|
||
| @Test | ||
| public void testResourceIdTester() throws Exception { | ||
| FileSystems.setDefaultConfigInWorkers(TestPipeline.testingPipelineOptions()); |
There was a problem hiding this comment.
I think you want to set the Configuration on PipelineOptions and not use TestPipeline.testingPipelineOptions here
| @@ -163,6 +166,12 @@ public void testGetFilename() throws Exception { | |||
| "xyz.txt"); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testResourceIdTester() throws Exception { | |||
| FileSystems.setDefaultConfigInWorkers(TestPipeline.testingPipelineOptions()); | |||
…ntations A first cut at some of the parts of the ResourceId spec.
* Drop empty authority always * Resolve directories
dhalperi
force-pushed
the
b2277-resourceid-tester
branch
from
1420d7f
to
f24ebb8
Compare
|
Still todo:
|
And document that it's not for users.
dhalperi
force-pushed
the
b2277-resourceid-tester
branch
from
f24ebb8
to
00c02ec
Compare
dhalperi
added a commit
to dhalperi/beam
that referenced
this pull request
May 12, 2017
fbb0de1 Remove '/' entirely from determining FileSystem scheme a6a5ff7 [BEAM-2277] Add ResourceIdTester and test existing ResourceId implementations ec956c8 Mark FileSystem and related as Experimental 15df211 [BEAM-2277] HadoopFileSystem: normalize implementation f3540d4 Rename FileSystems.setDefaultConfigInWorkers 3921163 Fix shading of guava testlib
asfgit
pushed a commit
that referenced
this pull request
May 12, 2017
fbb0de1 Remove '/' entirely from determining FileSystem scheme a6a5ff7 [BEAM-2277] Add ResourceIdTester and test existing ResourceId implementations ec956c8 Mark FileSystem and related as Experimental 15df211 [BEAM-2277] HadoopFileSystem: normalize implementation f3540d4 Rename FileSystems.setDefaultConfigInWorkers 3921163 Fix shading of guava testlib
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.