[
https://issues.apache.org/jira/browse/FLINK-10399?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16643568#comment-16643568
]
ASF GitHub Bot commented on FLINK-10399:
----------------------------------------
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:
us...@infra.apache.org
> Refactor ParameterTool#fromArgs
> -------------------------------
>
> Key: FLINK-10399
> URL: https://issues.apache.org/jira/browse/FLINK-10399
> Project: Flink
> Issue Type: Improvement
> Components: Client
> Affects Versions: 1.7.0
> Reporter: TisonKun
> Assignee: TisonKun
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.7.0
>
>
> {{ParameterTool#fromArgs}} uses a weird implement which flink developer would
> fail to parse it fast.
> The main problem is that, when parse args, we always try to get a key-value
> pair, but the implement iterate by a {{for}} loop, thus introduce weird
> flag/mutable variable and branches.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
