You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2018/10/09 14:55:56 UTC
[GitHub] asfgit closed pull request #6737: [FLINK-10399] Refactor
ParameterTool#fromArgs
asfgit closed pull request #6737: [FLINK-10399] Refactor ParameterTool#fromArgs
URL: https://github.com/apache/flink/pull/6737
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/flink-java/src/main/java/org/apache/flink/api/java/utils/ParameterTool.java b/flink-java/src/main/java/org/apache/flink/api/java/utils/ParameterTool.java
index 7518fa08fac..9adbcafe11e 100644
--- a/flink-java/src/main/java/org/apache/flink/api/java/utils/ParameterTool.java
+++ b/flink-java/src/main/java/org/apache/flink/api/java/utils/ParameterTool.java
@@ -68,81 +68,43 @@
* @return A {@link ParameterTool}
*/
public static ParameterTool fromArgs(String[] args) {
- Map<String, String> map = new HashMap<String, String>(args.length / 2);
-
- String key = null;
- String value = null;
- boolean expectValue = false;
- for (String arg : args) {
- // check for -- argument
- if (arg.startsWith("--")) {
- if (expectValue) {
- // we got into a new key, even though we were a value --> current key is one without value
- if (value != null) {
- throw new IllegalStateException("Unexpected state");
- }
- map.put(key, NO_VALUE_KEY);
- // key will be overwritten in the next step
- }
- key = arg.substring(2);
- expectValue = true;
- } // check for - argument
- else if (arg.startsWith("-")) {
- // we are waiting for a value, so this is a - prefixed value (negative number)
- if (expectValue) {
-
- if (NumberUtils.isNumber(arg)) {
- // negative number
- value = arg;
- expectValue = false;
- } else {
- if (value != null) {
- throw new IllegalStateException("Unexpected state");
- }
- // We waited for a value but found a new key. So the previous key doesnt have a value.
- map.put(key, NO_VALUE_KEY);
- key = arg.substring(1);
- expectValue = true;
- }
- } else {
- // we are not waiting for a value, so its an argument
- key = arg.substring(1);
- expectValue = true;
- }
+ final Map<String, String> map = new HashMap<>(args.length / 2);
+
+ int i = 0;
+ while (i < args.length) {
+ final String key;
+
+ if (args[i].startsWith("--")) {
+ key = args[i].substring(2);
+ } else if (args[i].startsWith("-")) {
+ key = args[i].substring(1);
} else {
- if (expectValue) {
- value = arg;
- expectValue = false;
- } else {
- throw new RuntimeException("Error parsing arguments '" + Arrays.toString(args) + "' on '" + arg + "'. Unexpected value. Please prefix values with -- or -.");
- }
+ throw new IllegalArgumentException(
+ String.format("Error parsing arguments '%s' on '%s'. Please prefix keys with -- or -.",
+ Arrays.toString(args), args[i]));
}
- if (value == null && key == null) {
- throw new IllegalStateException("Value and key can not be null at the same time");
- }
- if (key != null && value == null && !expectValue) {
- throw new IllegalStateException("Value expected but flag not set");
- }
- if (key != null && value != null) {
- map.put(key, value);
- key = null;
- value = null;
- expectValue = false;
- }
- if (key != null && key.length() == 0) {
- throw new IllegalArgumentException("The input " + Arrays.toString(args) + " contains an empty argument");
+ if (key.length() == 0) {
+ throw new IllegalArgumentException(
+ "The input " + Arrays.toString(args) + " contains an empty argument");
}
- if (key != null && !expectValue) {
+ i += 1; // try to find the value
+
+ if (i >= args.length) {
+ map.put(key, NO_VALUE_KEY);
+ } else if (NumberUtils.isNumber(args[i])) {
+ map.put(key, args[i]);
+ i += 1;
+ } else if (args[i].startsWith("--") || args[i].startsWith("-")) {
+ // the argument cannot be a negative number because we checked earlier
+ // -> the next argument is a parameter name
map.put(key, NO_VALUE_KEY);
- key = null;
- expectValue = false;
+ } else {
+ map.put(key, args[i]);
+ i += 1;
}
}
- if (key != null) {
- map.put(key, NO_VALUE_KEY);
- }
return fromMap(map);
}
@@ -497,7 +459,7 @@ public byte getByte(String key, byte defaultValue) {
// --------------- Internals
protected void addToDefaults(String key, String value) {
- String currentValue = defaultData.get(key);
+ final String currentValue = defaultData.get(key);
if (currentValue == null) {
if (value == null) {
value = DEFAULT_UNDEFINED;
@@ -520,7 +482,7 @@ protected void addToDefaults(String key, String value) {
* @return A {@link Configuration}
*/
public Configuration getConfiguration() {
- Configuration conf = new Configuration();
+ final Configuration conf = new Configuration();
for (Map.Entry<String, String> entry : data.entrySet()) {
conf.setString(entry.getKey(), entry.getValue());
}
@@ -559,7 +521,7 @@ public void createPropertiesFile(String pathToFile) throws IOException {
* @throws IOException If overwrite is not allowed and the file exists
*/
public void createPropertiesFile(String pathToFile, boolean overwrite) throws IOException {
- File file = new File(pathToFile);
+ final File file = new File(pathToFile);
if (file.exists()) {
if (overwrite) {
file.delete();
@@ -567,7 +529,7 @@ public void createPropertiesFile(String pathToFile, boolean overwrite) throws IO
throw new RuntimeException("File " + pathToFile + " exists and overwriting is not allowed");
}
}
- Properties defaultProps = new Properties();
+ final Properties defaultProps = new Properties();
defaultProps.putAll(this.defaultData);
try (final OutputStream out = new FileOutputStream(file)) {
defaultProps.store(out, "Default file created by Flink's ParameterUtil.createPropertiesFile()");
@@ -588,11 +550,11 @@ protected Object clone() throws CloneNotSupportedException {
* @return The Merged {@link ParameterTool}
*/
public ParameterTool mergeWith(ParameterTool other) {
- Map<String, String> resultData = new HashMap<>(data.size() + other.data.size());
+ final Map<String, String> resultData = new HashMap<>(data.size() + other.data.size());
resultData.putAll(data);
resultData.putAll(other.data);
- ParameterTool ret = new ParameterTool(resultData);
+ final ParameterTool ret = new ParameterTool(resultData);
final HashSet<String> requestedParametersLeft = new HashSet<>(data.keySet());
requestedParametersLeft.removeAll(unrequestedParameters);
diff --git a/flink-java/src/test/java/org/apache/flink/api/java/utils/AbstractParameterToolTest.java b/flink-java/src/test/java/org/apache/flink/api/java/utils/AbstractParameterToolTest.java
index 784a50eeb70..a5d2f8784d5 100644
--- a/flink-java/src/test/java/org/apache/flink/api/java/utils/AbstractParameterToolTest.java
+++ b/flink-java/src/test/java/org/apache/flink/api/java/utils/AbstractParameterToolTest.java
@@ -30,8 +30,6 @@
import java.io.IOException;
import java.util.Properties;
-import static org.junit.Assert.fail;
-
/**
* Base class for tests for {@link ParameterTool}.
*/
@@ -42,40 +40,39 @@
protected void validate(ParameterTool parameter) {
ClosureCleaner.ensureSerializable(parameter);
- validatePrivate(parameter);
+ internalValidate(parameter);
// -------- test behaviour after serialization ------------
- ParameterTool copy = null;
try {
byte[] b = InstantiationUtil.serializeObject(parameter);
- copy = InstantiationUtil.deserializeObject(b, getClass().getClassLoader());
+ final ParameterTool copy = InstantiationUtil.deserializeObject(b, getClass().getClassLoader());
+ internalValidate(copy);
} catch (Exception e) {
- fail();
+ e.printStackTrace();
+ Assert.fail(e.getMessage());
}
- validatePrivate(copy);
}
- private void validatePrivate(ParameterTool parameter) {
+ private void internalValidate(ParameterTool parameter) {
Assert.assertEquals("myInput", parameter.getRequired("input"));
Assert.assertEquals("myDefaultValue", parameter.get("output", "myDefaultValue"));
- Assert.assertEquals(null, parameter.get("whatever"));
+ Assert.assertNull(parameter.get("whatever"));
Assert.assertEquals(15L, parameter.getLong("expectedCount", -1L));
Assert.assertTrue(parameter.getBoolean("thisIsUseful", true));
Assert.assertEquals(42, parameter.getByte("myDefaultByte", (byte) 42));
Assert.assertEquals(42, parameter.getShort("myDefaultShort", (short) 42));
- Configuration config = parameter.getConfiguration();
+ final Configuration config = parameter.getConfiguration();
Assert.assertEquals(15L, config.getLong("expectedCount", -1L));
- Properties props = parameter.getProperties();
+ final Properties props = parameter.getProperties();
Assert.assertEquals("myInput", props.getProperty("input"));
- props = null;
// -------- test the default file creation ------------
try {
- String pathToFile = tmp.newFile().getAbsolutePath();
+ final String pathToFile = tmp.newFile().getAbsolutePath();
parameter.createPropertiesFile(pathToFile);
- Properties defaultProps = new Properties();
+ final Properties defaultProps = new Properties();
try (FileInputStream fis = new FileInputStream(pathToFile)) {
defaultProps.load(fis);
}
@@ -85,8 +82,8 @@ private void validatePrivate(ParameterTool parameter) {
Assert.assertTrue(defaultProps.containsKey("input"));
} catch (IOException e) {
- Assert.fail(e.getMessage());
e.printStackTrace();
+ Assert.fail(e.getMessage());
}
}
}
diff --git a/flink-java/src/test/java/org/apache/flink/api/java/utils/ParameterToolTest.java b/flink-java/src/test/java/org/apache/flink/api/java/utils/ParameterToolTest.java
index ccd472baaba..69f95de0013 100644
--- a/flink-java/src/test/java/org/apache/flink/api/java/utils/ParameterToolTest.java
+++ b/flink-java/src/test/java/org/apache/flink/api/java/utils/ParameterToolTest.java
@@ -57,9 +57,12 @@
// ----- Parser tests -----------------
- @Test(expected = RuntimeException.class)
- public void testIllegalArgs() {
- ParameterTool.fromArgs(new String[]{"berlin"});
+ @Test
+ public void testThrowExceptionIfParameterIsNotPrefixed() {
+ exception.expect(IllegalArgumentException.class);
+ exception.expectMessage("Error parsing arguments '[a]' on 'a'. Please prefix keys with -- or -.");
+
+ ParameterTool.fromArgs(new String[]{"a"});
}
@Test
@@ -100,13 +103,19 @@ public void testMultipleNoValMixed() {
Assert.assertTrue(parameter.has("f"));
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testEmptyVal() {
+ exception.expect(IllegalArgumentException.class);
+ exception.expectMessage("The input [--a, -b, --] contains an empty argument");
+
ParameterTool.fromArgs(new String[]{"--a", "-b", "--"});
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testEmptyValShort() {
+ exception.expect(IllegalArgumentException.class);
+ exception.expectMessage("The input [--a, -b, -] contains an empty argument");
+
ParameterTool.fromArgs(new String[]{"--a", "-b", "-"});
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services