Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4921#discussion_r148286896
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -83,82 +82,100 @@ public void add(Option option) throws
RequiredParametersException {
* <p>If any check fails, a RequiredParametersException is thrown
*
* @param parameterTool - parameters supplied by the user.
+ * @return New ParameterTool instance with default values set
* @throws RequiredParametersException if any of the specified checks
fail
*/
- public void applyTo(ParameterTool parameterTool) throws
RequiredParametersException {
+ public ParameterTool applyTo(ParameterTool parameterTool) throws
RequiredParametersException {
List<String> missingArguments = new LinkedList<>();
+
+ ParameterTool resultParameterTool = parameterTool;
for (Option o : data.values()) {
- if (parameterTool.data.containsKey(o.getName())) {
- if
(Objects.equals(parameterTool.data.get(o.getName()),
ParameterTool.NO_VALUE_KEY)) {
+ if (resultParameterTool.has(o.getName())) {
+ if
(Objects.equals(resultParameterTool.get(o.getName()),
ParameterTool.NO_VALUE_KEY)) {
// the parameter has been passed, but
no value, check if there is a default value
- checkAndApplyDefaultValue(o,
parameterTool.data);
+ resultParameterTool =
checkAndApplyDefaultValue(o, resultParameterTool);
} else {
// a value has been passed in the
parameterTool, now check if it adheres to all constraints
- checkAmbiguousValues(o,
parameterTool.data);
- checkIsCastableToDefinedType(o,
parameterTool.data);
- checkChoices(o, parameterTool.data);
+ checkAmbiguousValues(o,
resultParameterTool);
+ checkIsCastableToDefinedType(o,
resultParameterTool);
+ checkChoices(o, resultParameterTool);
}
} else {
// check if there is a default name or a value
passed for a possibly defined alternative name.
- if
(hasNoDefaultValueAndNoValuePassedOnAlternativeName(o, parameterTool.data)) {
+ resultParameterTool =
synchronizeAlternativeName(o, resultParameterTool);
+
+ if (!resultParameterTool.has(o.getName()) ||
Objects.equals(resultParameterTool.get(o.getName()),
ParameterTool.NO_VALUE_KEY)) {
missingArguments.add(o.getName());
}
}
}
if (!missingArguments.isEmpty()) {
throw new
RequiredParametersException(this.missingArgumentsText(missingArguments),
missingArguments);
}
+
+ return resultParameterTool;
}
// check if the given parameter has a default value and add it to the
passed map if that is the case
// else throw an exception
- private void checkAndApplyDefaultValue(Option o, Map<String, String>
data) throws RequiredParametersException {
- if (hasNoDefaultValueAndNoValuePassedOnAlternativeName(o,
data)) {
+ private ParameterTool checkAndApplyDefaultValue(Option o, ParameterTool
parameterTool) throws RequiredParametersException {
+ final ParameterTool resultParameterTool =
synchronizeAlternativeName(o, parameterTool);
+
+ if (!resultParameterTool.has(o.getName()) ||
Objects.equals(resultParameterTool.get(o.getName()),
ParameterTool.NO_VALUE_KEY)) {
throw new RequiredParametersException("No default value
for undefined parameter " + o.getName());
+ } else {
+ return resultParameterTool;
}
}
// check if the value in the given map which corresponds to the name of
the given option
// is castable to the type of the option (if any is defined)
- private void checkIsCastableToDefinedType(Option o, Map<String, String>
data) throws RequiredParametersException {
- if (o.hasType() &&
!o.isCastableToDefinedType(data.get(o.getName()))) {
+ private void checkIsCastableToDefinedType(Option o, ParameterTool
parameterTool) throws RequiredParametersException {
+ if (o.hasType() &&
!o.isCastableToDefinedType(parameterTool.get(o.getName()))) {
throw new RequiredParametersException("Value for
parameter " + o.getName() +
" cannot be cast to type " +
o.getType());
}
}
// check if the value in the given map which corresponds to the name of
the given option
// adheres to the list of given choices for the param in the options
(if any are defined)
- private void checkChoices(Option o, Map<String, String> data) throws
RequiredParametersException {
- if (o.getChoices().size() > 0 &&
!o.getChoices().contains(data.get(o.getName()))) {
- throw new RequiredParametersException("Value " +
data.get(o.getName()) +
+ private void checkChoices(Option o, ParameterTool parameterTool) throws
RequiredParametersException {
+ if (o.getChoices().size() > 0 &&
!o.getChoices().contains(parameterTool.get(o.getName()))) {
+ throw new RequiredParametersException("Value " +
parameterTool.get(o.getName()) +
" is not in the list of valid choices
for key " + o.getName());
}
}
// move value passed on alternative name to standard name or apply
default value if any defined
- // else return true to indicate parameter is 'really' missing
- private boolean
hasNoDefaultValueAndNoValuePassedOnAlternativeName(Option o, Map<String,
String> data)
- throws RequiredParametersException {
- if (o.hasAlt() && data.containsKey(o.getAlt())) {
- data.put(o.getName(), data.get(o.getAlt()));
+ // if any change was applied, then this method returns a new
ParameterTool instance with these
+ // changes. If not, then the passed ParameterTool instance will be
returned.
+ private ParameterTool synchronizeAlternativeName(Option o,
ParameterTool parameterTool) {
+ // TODO: Throw this all away!!!
+ if (o.hasAlt() && parameterTool.has(o.getAlt())) {
+ HashMap<String, String> newData = new
HashMap<>(parameterTool.toMap());
+ newData.put(o.getName(), parameterTool.get(o.getAlt()));
+
+ return ParameterTool.fromMap(newData);
} else {
if (o.hasDefaultValue()) {
- data.put(o.getName(), o.getDefaultValue());
+ HashMap<String, String> newData = new
HashMap<>(parameterTool.toMap());
--- End diff --
could we not instead create a copy at the start of applyTo and modify that?
---
