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.

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

Move ES_TMPDIR substitution into jvm options parser #47189

Conversation

jasontedor
Copy link
Member

This commit moves the ES_TMPDIR substitution that we do for JVM options into the JVM options parser itself. This solves a problem where the fact that the we do not make the substitution before ergonomics parsing can lead to the JVM that we start for computing the ergonomic values failing to start. Additionally, moving this substitution here enables us to simplify the shell scripts since we do not need to implement this there, and twice for Bash and Windows.

Closes #47133

This commit moves the ES_TMPDIR substitution that we do for JVM options
into the JVM options parser itself. This solves a problem where the fact
that the we do not make the substitution before ergonomics parsing can
lead to the JVM that we start for computing the ergonomic values failing
to start. Additionally, moving this substitution here enables us to
simplify the shell scripts since we do not need to implement this there,
and twice for Bash and Windows.
@jasontedor jasontedor added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v7.5.0 labels Sep 27, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member Author

@rjernst This is only a draft, I need to add tests, and actually test this on Windows (I haven't) so no need for a formal review, really looking for feedback on the direction I'm taking this.

@rjernst
Copy link
Member

rjernst commented Sep 27, 2019

The direction looks good.

@jasontedor jasontedor marked this pull request as ready for review September 27, 2019 16:25
@jasontedor
Copy link
Member Author

@rjernst This is ready for you.

@jasontedor
Copy link
Member Author

@rjernst Can you review?

Comment on lines 120 to 137
static List<String> substitutePlaceholders(final List<String> jvmOptions, Map<String, String> substitutions) {
final Map<String, String> placeholderSubstitutions =
substitutions.entrySet().stream().collect(Collectors.toMap(e -> "${" + e.getKey() + "}", Map.Entry::getValue));
final List<String> substitutedJvmOptions = new ArrayList<>(jvmOptions.size());
for (final String jvmOption : jvmOptions) {
if (jvmOption.matches(".*\\$\\{[^}]+\\}.*")) {
String actualJvmOption = jvmOption;
for (final Map.Entry<String, String> placeholderSubstitution : placeholderSubstitutions.entrySet()) {
actualJvmOption = actualJvmOption.replace(placeholderSubstitution.getKey(), placeholderSubstitution.getValue());
}
substitutedJvmOptions.add(actualJvmOption);
} else {
substitutedJvmOptions.add(jvmOption);
}
}
return substitutedJvmOptions;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This method can be simplified with use of mathce.appendReplacement method:

private static final Pattern SUBSTITUTION_PATTERN = Pattern.compile("\\$\\{([^}]+)}");

static List<String> substitutePlaceholders(final List<String> jvmOptions, Map<String, String> substitutions) {
    return jvmOptions.stream().map(jvmOption -> {
        StringBuilder sb = new StringBuilder();
        Matcher matcher = SUBSTITUTION_PATTERN.matcher(jvmOption);
        while (matcher.find()) {
            matcher.appendReplacement(sb, substitutions.get(matcher.group(1)));
        }
        matcher.appendTail(sb);
        return sb.toString();
    }).collect(Collectors.toList());
}

Or made faster (~2x) when you replace regex with indexOf:

static List<String> substitutePlaceholders(final List<String> jvmOptions, Map<String, String> substitutions) {
    final Map<String, String> placeholderSubstitutions =
            substitutions.entrySet().stream().collect(Collectors.toMap(e -> "${" + e.getKey() + "}", Map.Entry::getValue));
    return jvmOptions.stream().map(jvmOption -> {
        String actualJvmOption = jvmOption;
        int start = jvmOption.indexOf("${");
        if (start >= 0 && jvmOption.indexOf('}', start) > 0) {
            for (final Map.Entry<String, String> placeholderSubstitution : placeholderSubstitutions.entrySet()) {
                actualJvmOption = actualJvmOption.replace(placeholderSubstitution.getKey(), placeholderSubstitution.getValue());
            }
        }
        return actualJvmOption;
    }).collect(Collectors.toList());
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @probakowski, I pushed it.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM. I do think @przemek-grzedzielski 's alternative to use indexOf is simpler.

@jasontedor jasontedor merged commit bf2a5a0 into elastic:master Oct 4, 2019
jasontedor added a commit that referenced this pull request Oct 4, 2019
This commit moves the ES_TMPDIR substitution that we do for JVM options
into the JVM options parser itself. This solves a problem where the fact
that the we do not make the substitution before ergonomics parsing can
lead to the JVM that we start for computing the ergonomic values failing
to start. Additionally, moving this substitution here enables us to
simplify the shell scripts since we do not need to implement this there,
and twice for Bash and Windows.
@jasontedor jasontedor deleted the jvm-options-es-tmpdir-in-jvm-options-parser branch October 4, 2019 23:12
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >enhancement Team:Delivery Meta label for Delivery team v7.5.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JvmOptionsParser - cannot resolve enviroment variables
6 participants