-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Make the build version qualifier aware #32128
Conversation
And relax the check in build to only take the number into account
Pinging @elastic/es-core-infra |
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 left some initial thoughts.
baseVersion.getMajor(), | ||
baseVersion.getMinor(), | ||
baseVersion.getRevision(), | ||
System.getProperty("build.version_qualifier", "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.
This should not default to alpha1
. It should default to the empty string. In the production of our nightly snapshots we want to set this externally now and we will start with 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.
I think that this also means that we need to percolate this through to CI otherwise the builds will be looking for ML 7.0.0-SNAPSHOT artifacts which will not exist.
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.
One reason that I do not like the default of alpha1
and instead prefer this to be completely managed externally is that we can end up with the following sequence:
- today we are producing
alpha1
snapshots - at some point we will bump to producing
alpha2
snapshots - with this default, we would still be running
alpha1
in CI and that's confusing and wrong (wrong because we need it to bealpha2
to pick up the latest ML builds)
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 the confusion is that without any qualifier, we'll be producing 7.0.0-SNAPSHOT
which is not something we produce now AFAIK, as we go 7.0.0-alpha1-SNAPSHOT
-> e.x 7.0.0-rc3-SNAPSHOT
-> 7.0.0-rc3
-> 7.0.0
. In this context 7.0.0-SNAPSHOT
is a pre-alpha1 .
We need to check the Version
class in the build for this, since I don't think it compares like that.
It will a bit awkward to start with 7.0.0-SNAPSHOT
end with 7.0.0.
and have all the qualifiers in between, but I think it's the only way to have a default to no qualifier. That means CI will use this default while we are in development mode, and start using the alpha1
qualifier close to the release only. We don't want to force people building outside of CI to pass this, and knowing this always passing it in CI feels like to big of a difference to me. If we do this, a good way to percolate this to CI is to merge it like this, with alpha1
as default, then start changing it to empty in CI and eventually change the code to default to empty and remove the parameter from CI.
An alternative is to continue having the qualifier in version.properties
, but also allow for a property. This way the bump from -alpha1
to alpha2
all the way to rc3
will be a code change,
but if we are happy with rc3
our job to produce the final release will not do a code change but pass in the qualifier using the property to override the one in version.properties
. I think this might be a more straight forward approach as it avoids 7.0.0-SNAPSHOT
. We can even enforce that an empty qualifier can't be a snapshot. It's also easier to propagate and less disruptive overall.
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 am starting to come around to the view that we simply always produce unqualified builds, yet from time to time we will ship a qualified build with a progression of alpha1
, alpha2
, etc. as we head towards GA. This feels like the simplest approach and I am not seeing any major downsides? We would still need to bake the qualifier into the version ID when a qualified build is produced though.
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.
@jasontedor you mean to have CI always build a snapshot without a qualifier like 7.0.0-SNAPSHOT
and then when we want to ship something we would have one with a qualifier only then e.x 7.0.0-alpha1
so we'll have snapshots before and after the qualified build without distinguishing them. Snapshot and qualifier will become mutually exclusive.
I like it how that simplifies things, and I think it is ok since we either don't consider the qualifiers already ( e.x. we remove qualified versions from bwc tests ) or we are making changes to compare without the version qualifier ( e.x. when comparing build with Version.java
and when comparing plugin vs es version ). The only risk I can think of is that we will never run those qualified builds that we make time to time trough our testing. Is there already some sanity tests that runs on artifacts produced by the RM ? That being said, this isn't entirely new, it's just one way there could be differences between elasticsearch ci and RM and the version comparison code is easy to unit test to make sure it does the right thing.
public static final Version V_7_0_0_alpha1 = | ||
new Version(V_7_0_0_alpha1_ID, org.apache.lucene.util.Version.LUCENE_7_4_0); | ||
public static final Version CURRENT = V_7_0_0_alpha1; | ||
public static final int V_7_0_0_ID = 7000001; |
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.
This needs to be 7000099
.
public static final Version CURRENT = V_7_0_0_alpha1; | ||
public static final int V_7_0_0_ID = 7000001; | ||
public static final Version V_7_0_0 = new Version(V_7_0_0_ID, org.apache.lucene.util.Version.LUCENE_7_4_0); | ||
public static final Version CURRENT = V_7_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.
This requires some thought as this now depends on the build. I think that we might need to dynamically ascertain the value of CURRENT
based on the version and the qualifier (dynamically compute the ID).
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 we do that, we'll have to model the qualifier here again.
My understanding is that this code will not know about the qualifier,
in this respect I don't think there's any change, we used to have the version and qualifier hard coded here with matching in version.properties
, so if we no longer care about the qualifier here,
it pretty much stays the same regardless of the fact that the qualifier can be changed at build time.
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.
@jasontedor re-reading your comment, I think I misinterpreted it. Are you thinking about determining CURRENT based on the version in the jar ? Topically we would change this for an alpha and will not have to update it after that. We could make a change in a separate PR to assign this based on the manifest. let me know.
This needs more work around should we throw away the qualifier when comparing ? What are your thoughts @jasontedor ? |
@atorok I left my latest thoughts in a response to our inline discussion. I am eager to hear your thoughts. |
I updated the plugin builder plugin not to include the build qualifier in the plugin description so that it will correctly match at run-time. This is similar to what was done for snapshot and does not modify the server code. There are further implications of not including qualifiers in
All the caveats makes this logic difficult to grasp, hard and error prone to change, so I am wondering if this would be a good opportunity to try and simplify it. We could pas this PR with a flag and act from there right after it's merged so we don't increase this further. I'm thinking about having a meta file, that we always back-port everywhere. The bwc could enforce this when checking out older versions to build them. This would have information about every version ever released or consider (just an example does not match reality):
This would tell us that The first thing I would look at is to have a task that generates a
Pinging @hub-cap and @DaveCTurner , the authors of |
@jasontedor this is ready for another round of review with the caveat that |
@elasticmachine test this please |
I have nothing concrete to add here. I found it fairly mind-bending to try and encode our versioning policies and release history in code that is itself versioned via those policies, and what I originally wrote in There is tension between (a) builds need to know about which versions have really been released vs (b) builds should be as reproducible as possible. I think being explicit is a good idea, along with automation to verify that the explicit description is consistent with reality and ideally to help update the explicit description as reality 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.
I don't think this change should be modifying how SNAPSHOT builds work. Snapshots are independent of the version number, except for maven and the resulting artifact names. It was removed a long time ago from the Version code, and a very intentional decision to make the build produce snapshots by default. Why isn't the qualifier just the alpha/beta/rc portion of the version?
@rjernst snapshots will still be the default, alpha/beta etc is the qualifier, and snapshot or not is not part of that. I was only proposing we don't allow snapshots with a qualifier to make things less confusing, |
Every team would need to do the same for this to work with release manager, and it will make it very hard to keep release manager working if different teams implement this functionality at different times (which they will). I think we need to stick with "alpha1" as a qualifier for 7.0.0 snapshot builds at least until every team has made their build qualifier-aware. Even after that any changes would need to be agreed across dev. For example, Kibana CI tests use Elasticsearch so we'd need consistency over whether snapshots have qualifiers in Kibana CI. |
Indeed, as @droberts195 points out, we need to preserve that snapshots can be qualified for the time being. This is so that this change can be merged, the rest of the stack can proceed iteratively, and with corresponding internal changes to release-manager, we can continue to produce 7.0.0-alpha1-SNAPSHOTs until the remainder of the stack is ready for qualifiers to be controlled by release-manager. At that point, we would then be able to start producing pure 7.0.0-SNAPSHOTs and only produce qualified builds at pre-release times. |
@jasontedor @rjernst this is ready for review |
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 have few questions.
return items | ||
.tailSet(Version.fromString("${version.major}.0.0")) | ||
.headSet(Version.fromString("${version.major + 1}.0.0")) | ||
.count { it.suffix.isEmpty() } // count only non suffix'd versions as actual versions that may be released | ||
.count { it.qualifier.isEmpty() } // count only non qualifier'd versions as actual versions that may be released |
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.
nit: qualifier'd -> qualified
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.
fixed, thanks
Objects.requireNonNull(major, "major version can't be null"); | ||
Objects.requireNonNull(minor, "minor version can't be null"); | ||
Objects.requireNonNull(revision, "revision version can't be null"); | ||
this.major = major; | ||
this.minor = minor; | ||
this.revision = revision; | ||
this.snapshot = snapshot; | ||
this.suffix = suffix == null ? "" : suffix; | ||
if (qualifier == null || qualifier.isEmpty()) { |
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 do we need this leniency? The semantics should be either no qualifier is null, or empty string, but not either.
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 it's because the instance was constructed with null most of the time but, we mostly get the qualifier to compare against it, so having ""
instead is more convenient and readable.
We could disallow nulls in the constructor, I would clean this up as part of #32872
} else if (qualifier.startsWith("-")) { | ||
this.qualifier = qualifier; | ||
} else { | ||
this.qualifier = "-" + qualifier; |
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.
Do we need to do the formatting here (prefixing with dash), or could this be done when printed?
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.
We used to store it with a dash so existing code expected it, but I didn't want to add have it in the property, so this takes care of that. I think the cleanest way would be to have an enum for the qualifier.
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.
An enum won't work since these are numbered (or it would require separate variable to keep track of the number). But I don't see why the leniency needs to exist here to add the dash if it doesn't exist. I think the qualifier should always have no dash internally, just like we don't have dot prefixes on the numeric parts of the version.
revision == other.revision; | ||
} | ||
|
||
public Version dropQualifier() { |
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.
nit: can we use the "without" terminology? I think this better matches other code like the builders for java time stuff having egwithTimeZone
. Drop implies mutating the current object.
if (this.suffix.isEmpty()) { | ||
// no suffix will be considered smaller, uncomment to change that | ||
if (this.qualifier.isEmpty()) { | ||
// no qualifier will be considered smaller, uncomment to change that | ||
// suffixOffset = 100; |
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.
What is the purpose of having this commented out conditional block? With the new rules, we shouldn't ever be comparing a qualified build against each other or a non qualified build, since qualified and non qualified of the same version have the exact same bwc behavior.
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 added this comment some time ago when I discovered that 7.0.0
would be considered smaller than 7.0.0-alpha1
which is incorrect, but changing that caused failures in VersionCollection
and I didn't want to dig deeper at that time so just left this comment as a reminder.
The qualifier will not be relevant indeed for bwc behavior, but it's still relevant for the build, e.x. we still want to use an beta over an alpha if available so the build still has to order them.
I'm hoping that with the metadata format in #32872 the proper ordering will be clearer and the implementation will clean up as a result as well.
I think these are simple things, yet hard to get right with harsh consequences if they are not right, I'm reluctant to change them without the visualization described in #32872 to be able to better reason about 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.
we still want to use an beta over an alpha
There shouldn't be anything needing to choose a beta over an alpha? There should be nothing using any qualified build to check bwc.
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 removed the commented code to avoid confusion.
To make sure my understanding is correct: we will need to consider qualifier in the build, i.e. so we don't BWC test against an alpha if a beta is already available. This is for build only, server will not know of qualifier, so it has no way to decide based on 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.
I don't know what you mean by "consider qualifier in the build". There should be no attempt to test bwc of any qualified version, so I don't believe the build needs to know about it in our build Version, since that class is all about which versions we bwc test against.
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 version class is now used more broadly, including in figuring out dependencies ( as it's string value is set as project.version which is then used when considering dependencies ).
My understanding was that in BWC testing we will want to pic up alpha
, beta
etc as these become available, I'm starting to realize that since we don't offer guarantees we won't have to test with those and would just test against the latest snapshot and be ok.
I don't think we should remove the qualifier from the version right now, we should eventually better express requirements for a version used in bwc, perhaps as part of #32872.
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 version class is now used more broadly, including in figuring out dependencies ( as it's string value is set as project.version which is then used when considering dependencies ).
There shouldn't be any dependencies on a qualified version, so there should be no need to serialize it into a string value.
I don't think we should remove the qualifier from the version right now, we should eventually better express requirements for a version used in bwc
Why would we keep something around that is unused? I think it only adds to confusion in a class that is already difficult to under (our gradle's Version class).
@elasticmachine test this please |
baseVersion.getMinor(), | ||
baseVersion.getRevision(), | ||
// TODO: Change default to "" after CI "warm-up" by adding -Dbuild.version_qualifier="" to ML jobs to produce | ||
// a snapshot. |
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.
We now have the ability to set the ML C++ version qualifier when kicking off a build, so if it makes life easier for you I can create a one-off build of ML C++ 7.0.0-SNAPSHOT artifacts now so that the PR build can pass with a default qualifier of ""
. Then ping me when you merge this PR and I'll change the value for the periodic ML C++ builds to be no qualifier too.
@rjernst thank you! I get it now. The qualifier and snapshot is now completely removed from the version class. We still have to deal with them in a few places because the build will reference them like that in places like dependencies, the distributions will have them etc. |
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.
Thanks @atorok, this looks better. One minor question: why does VersionProperties.getElasticsearch() exist? I don't see a reason to keep a variant returning Version and another returning String?
@rjernst The |
@rjernst The more I look at the tests and the changes required to make this right, the less confident I feel about these changes and that I don't break any of the bwc tests. I think the logic is fairly complex and the implementation is spread across different places which makes it hard to understand and get right. I would rather revert to having snapshot and qualifier in |
I'm closing this PR as it has grown too much in favor of multiple smaller changes starting with #34050. |
-Dbuild.snapshot
so the full version string is computed ). The versions are read fromVersionProperties
anyhow and never directly.Version.java
, disregarding the qualifier since it will never be added toVersion.java
version.properties
andVersion.java
. All the updates in other java files are automated refactoring.