Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BEAM-59] Beam FileSystem.setDefaultConfig: remove scheme from the signature. #1826

Closed
wants to merge 2 commits into from
Closed

[BEAM-59] Beam FileSystem.setDefaultConfig: remove scheme from the signature. #1826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
974975e
[BEAM-59] Beam FileSystem.setDefaultConfig: remove scheme from the si…
peihe Jan 24, 2017
fb54273
fixup! addressed comments.
peihe Jan 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 10 additions & 21 deletions sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileSystems.java
View file
Open in desktop
Expand Up @@ -17,7 +17,6 @@
*/
package org.apache.beam.sdk.io;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.annotations.VisibleForTesting;
Expand Down Expand Up @@ -53,6 +52,8 @@ public class FileSystems {
private static final Map<String, FileSystemRegistrar> SCHEME_TO_REGISTRAR =
new ConcurrentHashMap<>();

private static PipelineOptions defaultConfig;

private static final Map<String, PipelineOptions> SCHEME_TO_DEFAULT_CONFIG =
new ConcurrentHashMap<>();

Expand All @@ -78,37 +79,25 @@ private static void loadFileSystemRegistrars() {
}

/**
* Sets the default configuration to be used with a {@link FileSystemRegistrar} for the provided
* {@code scheme}.
* Sets the default configuration in workers.
*
* <p>Syntax: <pre>scheme = alpha *( alpha | digit | "+" | "-" | "." )</pre>
* Upper case letters are treated as the same as lower case letters.
* <p>It will be used in {@link FileSystemRegistrar FileSystemRegistrars} for all schemes.
*/
public static void setDefaultConfig(String scheme, PipelineOptions options) {
String lowerCaseScheme = checkNotNull(scheme, "scheme").toLowerCase();
checkArgument(
URI_SCHEME_PATTERN.matcher(lowerCaseScheme).matches(),
String.format("Scheme: [%s] doesn't match URI syntax: %s",
lowerCaseScheme, URI_SCHEME_PATTERN.pattern()));
checkArgument(
SCHEME_TO_REGISTRAR.containsKey(lowerCaseScheme),
String.format("No FileSystemRegistrar found for scheme: [%s].", lowerCaseScheme));
SCHEME_TO_DEFAULT_CONFIG.put(lowerCaseScheme, checkNotNull(options, "options"));
}

@VisibleForTesting
static PipelineOptions getDefaultConfig(String scheme) {
return SCHEME_TO_DEFAULT_CONFIG.get(scheme.toLowerCase());
public static void setDefaultConfigInWorkers(PipelineOptions options) {
defaultConfig = checkNotNull(options, "options");
}

/**
* Internal method to get {@link FileSystem} for {@code spec}.
*/
@VisibleForTesting
static FileSystem getFileSystemInternal(URI uri) {
checkNotNull(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: checkState(defaultConfig != null to throw the proper exception type. But, I can fix this in the merge if LGTY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM

Thanks!

defaultConfig,
"Expect the runner have called setDefaultConfigInWorkers().");
String lowerCaseScheme = (uri.getScheme() != null
? uri.getScheme().toLowerCase() : LocalFileSystemRegistrar.LOCAL_FILE_SCHEME);
return getRegistrarInternal(lowerCaseScheme).fromOptions(getDefaultConfig(lowerCaseScheme));
return getRegistrarInternal(lowerCaseScheme).fromOptions(defaultConfig);
}

/**
Expand Down
33 changes: 4 additions & 29 deletions sdks/java/core/src/test/java/org/apache/beam/sdk/io/FileSystemsTest.java
View file
Open in desktop
Expand Up @@ -17,15 +17,14 @@
*/
package org.apache.beam.sdk.io;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;

import com.google.common.collect.Sets;
import java.net.URI;
import javax.annotation.Nullable;
import org.apache.beam.sdk.options.PipelineOptions;
import org.apache.beam.sdk.options.PipelineOptionsFactory;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Expand All @@ -41,33 +40,9 @@ public class FileSystemsTest {
@Rule
public ExpectedException thrown = ExpectedException.none();

@Test
public void testSetDefaultConfig() throws Exception {
PipelineOptions first = PipelineOptionsFactory.create();
PipelineOptions second = PipelineOptionsFactory.create();
FileSystems.setDefaultConfig("file", first);
assertEquals(first, FileSystems.getDefaultConfig("file"));
assertEquals(first, FileSystems.getDefaultConfig("FILE"));

FileSystems.setDefaultConfig("FILE", second);
assertNotEquals(first, FileSystems.getDefaultConfig("file"));
assertNotEquals(first, FileSystems.getDefaultConfig("FILE"));
assertEquals(second, FileSystems.getDefaultConfig("file"));
assertEquals(second, FileSystems.getDefaultConfig("FILE"));
}

@Test
public void testSetDefaultConfigNotFound() throws Exception {
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("No FileSystemRegistrar found for scheme: [gs-s3].");
FileSystems.setDefaultConfig("gs-s3", PipelineOptionsFactory.create());
}

@Test
public void testSetDefaultConfigInvalidScheme() throws Exception {
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("Scheme: [gs:] doesn't match URI syntax");
FileSystems.setDefaultConfig("gs:", PipelineOptionsFactory.create());
@Before
public void setup() {
FileSystems.setDefaultConfigInWorkers(PipelineOptionsFactory.create());
}

@Test
Expand Down