-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Update version to 7.0.0-alpha1 #25876
Conversation
This commit updates the version for master to 7.0.0-alpha1. It also adds the 6.1 version constant, and fixes many tests, as well as marking some as awaits fix.
I just pushed this branch to elastic/elasticsearch, hopefully someone can take this over as I am out the next 2 days and there are still lingering test failures. |
@rjernst I don't see any branch on |
Yes please do. Sorry I thought I had pushed it. |
In 6.x we had to serialize the 2 byte marker for the transport address type for BWC. Now in 6.x this doesn't exist and we write 2 bytes less when we serialize a transport address to a min compat node. Closes elastic#25893
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as far as I can see.
@@ -30,6 +30,7 @@ | |||
import org.elasticsearch.common.lucene.all.AllTermQuery; | |||
import org.elasticsearch.common.settings.Settings; | |||
import org.elasticsearch.common.xcontent.XContentBuilder; | |||
import org.elasticsearch.common.xcontent.json.JsonXContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this? I don't see it being used.
@@ -139,7 +138,7 @@ public void testSnapshotDeletionsInProgressSerialization() throws Exception { | |||
|
|||
// serialize with old version | |||
outStream = new BytesStreamOutput(); | |||
outStream.setVersion(Version.CURRENT.minimumIndexCompatibilityVersion()); | |||
outStream.setVersion(Version.V_5_0_0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why V_5_0_0? we should never serialize to such a node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah so I think this test can go away but I didn't wanna do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. I left a few minors that can be followups and I've marked one file as beyond my ability to properly review so I'd like another set of eyes on it.
ext.projectSubstitutions["org.elasticsearch.distribution.deb:elasticsearch:${indexCompatVersions[-2]}"] = ':distribution:bwc-release-snapshot' | ||
ext.projectSubstitutions["org.elasticsearch.distribution.rpm:elasticsearch:${indexCompatVersions[-2]}"] = ':distribution:bwc-release-snapshot' | ||
ext.projectSubstitutions["org.elasticsearch.distribution.zip:elasticsearch:${indexCompatVersions[-2]}"] = ':distribution:bwc-release-snapshot' | ||
if (indexCompatVersions.size() > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
if (currentVersion != foundVersion) { | ||
if (currentVersion != foundVersion | ||
&& (major == prevMajor || major == currentVersion.major) | ||
&& (versions.isEmpty() || versions.last() != foundVersion)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this adds all the betas and rcs are wire and index compatible versions. I think, at least in the end, we want them to only be index and wire compatible if we only have only have them. Otherwise we'll be testing compatibility against betas while we're well into 6.0. Is this a thing we should handle in a followup?
@@ -62,7 +63,7 @@ | |||
public class FullClusterRestartIT extends ESRestTestCase { | |||
private final boolean runningAgainstOldCluster = Booleans.parseBoolean(System.getProperty("tests.is_old_cluster")); | |||
private final Version oldClusterVersion = Version.fromString(System.getProperty("tests.old_cluster_version")); | |||
private final boolean supportsLenientBooleans = oldClusterVersion.onOrAfter(Version.V_6_0_0_alpha1); | |||
private final boolean supportsLenientBooleans = oldClusterVersion.before(Version.V_6_0_0_alpha1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@@ -213,19 +213,20 @@ public void testSeqNoCheckpoints() throws Exception { | |||
int numDocs = 0; | |||
final int numberOfInitialDocs = 1 + randomInt(5); | |||
logger.info("indexing [{}] docs initially", numberOfInitialDocs); | |||
numDocs += indexDocs(index, 0, numberOfInitialDocs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like someone else to have a look at this file's changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ywelsch did look at it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public static final Version V_6_0_0_alpha1 = Version.fromString("6.0.0-alpha1"); | ||
public static final Version V_6_0_0_alpha2 = Version.fromString("6.0.0-alpha2"); | ||
public static final Version V_6_0_0_beta1 = Version.fromString("6.0.0-beta1"); | ||
public static final Version CURRENT = V_6_0_0_beta1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of these tests was to not have to change them when we upgraded unless we found a new kind of layout. So maybe we don't have to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking if you have to change this test at all. When I wrote the test I used all the Version.fromString
so that you wouldn't have to change the tests as the versions marched along. I intended for the test to stay the same rather than mirror the current branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it - I fixed it
// Because things blow up all over the place if the minimum compatible version isn't released. | ||
// We'll fix this very, very soon. But for now, this hack. | ||
// end big horrible hack | ||
Version minimumCompatibleVersion = Version.CURRENT.minimumCompatibilityVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome.
@s1monw if you bump the version today, could you also bump the version here and here. I didn't want to backported it before running tests for a bit. So, it is currently only merged into master and if you start running 7.0.0 upgrade from 6.0.0 test, it might fail since the 6.0 branch doesn't have it. I will backport everything on Monday or over the weekend and change the version appropriately. |
This reverts commit ce8333b.
@imotov I merged master into this and bumped the version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pinging @elastic/es-core-infra |
This commit updates the version for master to 7.0.0-alpha1. It also adds
the 6.1 version constant, and fixes many tests, as well as marking some
as awaits fix.