lasdf1234 commented on code in PR #13134:
URL: https://github.com/apache/gravitino/pull/13134#discussion_r4013881469
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -947,17 +945,91 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
String key = matcher.group(1);
String replacement = replacements.get(key);
if (replacement != null) {
- matcher.appendReplacement(result, replacement);
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
} else {
// If no replacement is found, keep the placeholder as is
- matcher.appendReplacement(result, matcher.group(0));
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(matcher.group(0)));
}
}
matcher.appendTail(result);
return result.toString();
}
+ /**
+ * Drop blank / unresolved optional template arguments after placeholder
substitution.
+ *
+ * <p>Built-in templates always list optional flags as {@code --flag} +
{@code {{placeholder}}}.
+ * When the job conf omits that key or supplies an empty value, leaving the
flag in the command
+ * produces dangling arguments such as {@code --updater-options
--spark-conf}. This method
+ * removes:
+ *
+ * <ul>
+ * <li>blank tokens
+ * <li>tokens that are still an entire unresolved {@code {{placeholder}}}
+ * <li>{@code --flag} pairs whose following value is blank or an
unresolved placeholder
+ * </ul>
+ *
+ * @param arguments arguments after {@link #replacePlaceholder(String, Map)}
+ * @return compacted argument list suitable for process execution
+ */
+ @VisibleForTesting
+ static List<String> omitEmptyArguments(List<String> arguments) {
+ if (arguments == null || arguments.isEmpty()) {
+ return arguments;
+ }
+
+ List<String> result = new ArrayList<>(arguments.size());
+ for (int i = 0; i < arguments.size(); i++) {
+ String arg = arguments.get(i);
+ if (isUnresolvedOptionalValue(arg)) {
+ continue;
+ }
+
+ if (arg.startsWith("--") && i + 1 < arguments.size()) {
+ String next = arguments.get(i + 1);
+ if (!next.startsWith("--") && isUnresolvedOptionalValue(next)) {
+ i++;
+ continue;
+ }
+ }
+
+ result.add(arg);
+ }
+ return result;
+ }
+
+ /**
+ * Resolves optional template maps such as {@code environments}. Entries
whose keys or values are
+ * blank or still an unresolved {@code {{placeholder}}} after substitution
are dropped so
+ * unauthenticated / optional credentials do not become literal placeholder
strings.
+ *
+ * <p>{@code arguments} use {@link #omitEmptyArguments(List)}; {@code
customFields} still keep
+ * unresolved placeholders as literal text.
+ *
+ * @param source template map before substitution
+ * @param jobConf replacement values
+ * @return resolved map without blank or unresolved optional entries
+ */
+ private static Map<String, String> omitUnresolvedTemplateMap(
+ Map<String, String> source, Map<String, String> jobConf) {
+ Map<String, String> resolved = new LinkedHashMap<>();
+ for (Map.Entry<String, String> entry : source.entrySet()) {
+ String key = replacePlaceholder(entry.getKey(), jobConf);
+ String value = replacePlaceholder(entry.getValue(), jobConf);
+ if (isUnresolvedOptionalValue(key) || isUnresolvedOptionalValue(value)) {
+ continue;
+ }
+ resolved.put(key, value);
+ }
+ return resolved;
+ }
+
+ @VisibleForTesting
+ static boolean isUnresolvedOptionalValue(String value) {
+ return StringUtils.isBlank(value) ||
PLACEHOLDER_PATTERN.matcher(value).matches();
Review Comment:
Thank you very much for your review.
Half of it still contains {{...}} (such as prefix-{{x}}) → Immediately
report an error and do not pass it to the process in its original form.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -947,17 +945,91 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
String key = matcher.group(1);
String replacement = replacements.get(key);
if (replacement != null) {
- matcher.appendReplacement(result, replacement);
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
} else {
// If no replacement is found, keep the placeholder as is
- matcher.appendReplacement(result, matcher.group(0));
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(matcher.group(0)));
}
}
matcher.appendTail(result);
return result.toString();
}
+ /**
+ * Drop blank / unresolved optional template arguments after placeholder
substitution.
+ *
+ * <p>Built-in templates always list optional flags as {@code --flag} +
{@code {{placeholder}}}.
+ * When the job conf omits that key or supplies an empty value, leaving the
flag in the command
+ * produces dangling arguments such as {@code --updater-options
--spark-conf}. This method
+ * removes:
+ *
+ * <ul>
+ * <li>blank tokens
+ * <li>tokens that are still an entire unresolved {@code {{placeholder}}}
+ * <li>{@code --flag} pairs whose following value is blank or an
unresolved placeholder
+ * </ul>
+ *
+ * @param arguments arguments after {@link #replacePlaceholder(String, Map)}
+ * @return compacted argument list suitable for process execution
+ */
+ @VisibleForTesting
+ static List<String> omitEmptyArguments(List<String> arguments) {
+ if (arguments == null || arguments.isEmpty()) {
+ return arguments;
+ }
+
+ List<String> result = new ArrayList<>(arguments.size());
+ for (int i = 0; i < arguments.size(); i++) {
+ String arg = arguments.get(i);
+ if (isUnresolvedOptionalValue(arg)) {
+ continue;
+ }
+
+ if (arg.startsWith("--") && i + 1 < arguments.size()) {
+ String next = arguments.get(i + 1);
+ if (!next.startsWith("--") && isUnresolvedOptionalValue(next)) {
+ i++;
+ continue;
+ }
+ }
+
+ result.add(arg);
+ }
+ return result;
+ }
+
+ /**
+ * Resolves optional template maps such as {@code environments}. Entries
whose keys or values are
+ * blank or still an unresolved {@code {{placeholder}}} after substitution
are dropped so
+ * unauthenticated / optional credentials do not become literal placeholder
strings.
+ *
+ * <p>{@code arguments} use {@link #omitEmptyArguments(List)}; {@code
customFields} still keep
+ * unresolved placeholders as literal text.
+ *
+ * @param source template map before substitution
+ * @param jobConf replacement values
+ * @return resolved map without blank or unresolved optional entries
+ */
+ private static Map<String, String> omitUnresolvedTemplateMap(
+ Map<String, String> source, Map<String, String> jobConf) {
+ Map<String, String> resolved = new LinkedHashMap<>();
+ for (Map.Entry<String, String> entry : source.entrySet()) {
+ String key = replacePlaceholder(entry.getKey(), jobConf);
+ String value = replacePlaceholder(entry.getValue(), jobConf);
+ if (isUnresolvedOptionalValue(key) || isUnresolvedOptionalValue(value)) {
+ continue;
+ }
+ resolved.put(key, value);
Review Comment:
Thank you very much for your review.
This issue has been fixed. Before executing the "put" operation, we will
first check if the key already exists. If it does, an error will be thrown;
silent overwriting will no longer be performed.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -947,17 +945,91 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
String key = matcher.group(1);
String replacement = replacements.get(key);
if (replacement != null) {
- matcher.appendReplacement(result, replacement);
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
} else {
// If no replacement is found, keep the placeholder as is
- matcher.appendReplacement(result, matcher.group(0));
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(matcher.group(0)));
}
}
matcher.appendTail(result);
return result.toString();
}
+ /**
+ * Drop blank / unresolved optional template arguments after placeholder
substitution.
+ *
+ * <p>Built-in templates always list optional flags as {@code --flag} +
{@code {{placeholder}}}.
+ * When the job conf omits that key or supplies an empty value, leaving the
flag in the command
+ * produces dangling arguments such as {@code --updater-options
--spark-conf}. This method
+ * removes:
+ *
+ * <ul>
+ * <li>blank tokens
+ * <li>tokens that are still an entire unresolved {@code {{placeholder}}}
+ * <li>{@code --flag} pairs whose following value is blank or an
unresolved placeholder
+ * </ul>
+ *
+ * @param arguments arguments after {@link #replacePlaceholder(String, Map)}
+ * @return compacted argument list suitable for process execution
+ */
+ @VisibleForTesting
+ static List<String> omitEmptyArguments(List<String> arguments) {
+ if (arguments == null || arguments.isEmpty()) {
+ return arguments;
+ }
+
+ List<String> result = new ArrayList<>(arguments.size());
+ for (int i = 0; i < arguments.size(); i++) {
+ String arg = arguments.get(i);
+ if (isUnresolvedOptionalValue(arg)) {
Review Comment:
Thank you very much for your review.
1. Regarding the first issue, no more random deletions will be made; only
"name matching --flag + {{x}}" will be omitted, while the bare ones will be
retained.
2. Regarding the second issue, the case where --verbose + {{unset_flag}} was
mistakenly deleted has been fixed: if the names do not match, it will not be
regarded as a pair, and both will be kept.
3. Regarding the third issue, it has been changed to
Preconditions.checkNotNull(arguments), prohibiting null input parameters, and
no longer returning null. It leans towards fail-fast.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
