Skip to content

Speed up Version Checks #62216

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

Merged
merged 3 commits into from
Sep 10, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 55 additions & 28 deletions server/src/main/java/org/elasticsearch/Version.java
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ public class Version implements Comparable<Version>, ToXContentFragment {
assert CURRENT.luceneVersion.equals(org.apache.lucene.util.Version.LATEST) : "Version must be upgraded to ["
+ org.apache.lucene.util.Version.LATEST + "] is still set to [" + CURRENT.luceneVersion + "]";

builder.put(V_EMPTY_ID, V_EMPTY);
idToVersion = builder.build();
}

Expand All @@ -129,36 +130,36 @@ public static Version readVersion(StreamInput in) throws IOException {
}

public static Version fromId(int id) {
if (idToVersion.containsKey(id)) {
return idToVersion.get(id);
final Version known = idToVersion.get(id);
if (known != null) {
return known;
}
switch (id) {
case V_EMPTY_ID:
return V_EMPTY;
default:
// We need at least the major of the Lucene version to be correct.
// Our best guess is to use the same Lucene version as the previous
// version in the list, assuming that it didn't change.
List<Version> versions = DeclaredVersionsHolder.DECLARED_VERSIONS;
Version tmp = new Version(id, org.apache.lucene.util.Version.LATEST);
int index = Collections.binarySearch(versions, tmp);
if (index < 0) {
index = -2 - index;
} else {
assert false : "Version [" + tmp + "] is declared but absent from the switch statement in Version#fromId";
}
final org.apache.lucene.util.Version luceneVersion;
if (index == -1) {
// this version is older than any supported version, so we
// assume it is the previous major to the oldest Lucene version
// that we know about
luceneVersion = org.apache.lucene.util.Version.fromBits(
versions.get(0).luceneVersion.major - 1, 0, 0);
} else {
luceneVersion = versions.get(index).luceneVersion;
}
return new Version(id, luceneVersion);
return fromIdSlow(id);
}

private static Version fromIdSlow(int id) {
// We need at least the major of the Lucene version to be correct.
// Our best guess is to use the same Lucene version as the previous
// version in the list, assuming that it didn't change.
List<Version> versions = DeclaredVersionsHolder.DECLARED_VERSIONS;
Version tmp = new Version(id, org.apache.lucene.util.Version.LATEST);
int index = Collections.binarySearch(versions, tmp);
if (index < 0) {
index = -2 - index;
} else {
assert false : "Version [" + tmp + "] is declared but absent from the switch statement in Version#fromId";
}
final org.apache.lucene.util.Version luceneVersion;
if (index == -1) {
// this version is older than any supported version, so we
// assume it is the previous major to the oldest Lucene version
// that we know about
luceneVersion = org.apache.lucene.util.Version.fromBits(
versions.get(0).luceneVersion.major - 1, 0, 0);
} else {
luceneVersion = versions.get(index).luceneVersion;
}
return new Version(id, luceneVersion);
}

/**
Expand Down Expand Up @@ -285,6 +286,14 @@ private static class DeclaredVersionsHolder {
static final List<Version> DECLARED_VERSIONS = Collections.unmodifiableList(getDeclaredVersions(Version.class));
}

// lazy initialized because we don't yet have the declared versions ready when instantiating the cached Version
// instances
private Version minCompatVersion;
Copy link
Member

Choose a reason for hiding this comment

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

technically shouldn't we declare these as volatile?

Copy link
Member Author

Choose a reason for hiding this comment

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

technically shouldn't we declare these as volatile?

I figured no because we don't actively deduplicate this value so if we recompute it a few times (and realistically this is probably almost never going to happen) at startup it doesn't hurt but we pay the overhead in method size for volatile forever. But this was coded wrong even under that assumption now that I look at it again. I pushed 153763a so that it's safe even if out of order reads were to ever occur.


// lazy initialized because we don't yet have the declared versions ready when instantiating the cached Version
// instances
private Version minIndexCompatVersion;

/**
* Returns the minimum compatible version based on the current
* version. Ie a node needs to have at least the return version in order
Expand All @@ -293,6 +302,15 @@ private static class DeclaredVersionsHolder {
* is a beta or RC release then the version itself is returned.
*/
public Version minimumCompatibilityVersion() {
Version res = minCompatVersion;
if (res == null) {
res = computeMinCompatVersion();
minCompatVersion = res;
}
return res;
}

private Version computeMinCompatVersion() {
if (major == 6) {
// force the minimum compatibility for version 6 to 5.6 since we don't reference version 5 anymore
return Version.fromId(5060099);
Expand Down Expand Up @@ -324,6 +342,15 @@ public Version minimumCompatibilityVersion() {
* code that is used to read / write file formats like transaction logs, cluster state, and index metadata.
*/
public Version minimumIndexCompatibilityVersion() {
Version res = minIndexCompatVersion;
if (res == null) {
res = computeMinIndexCompatVersion();
minIndexCompatVersion = res;
}
return res;
}

private Version computeMinIndexCompatVersion() {
final int bwcMajor;
if (major == 5) {
bwcMajor = 2; // we jumped from 2 to 5
Expand Down