[
https://issues.apache.org/jira/browse/SOLR-18432?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113566#comment-18113566
]
Jan Høydahl commented on SOLR-18432:
------------------------------------
While I have not experienced these as issues they all sound like improvements
worth doing.
And yea, I'm quite shocked :)
> SolrVersion problems with prerelease (SNAPSHOT)
> -----------------------------------------------
>
> Key: SOLR-18432
> URL: https://issues.apache.org/jira/browse/SOLR-18432
> Project: Solr
> Issue Type: Bug
> Reporter: David Smiley
> Priority: Major
>
> {{SolrVersion.LATEST_STRING}} is a hardcoded constant (rewritten by
> {{{}dev-tools/scripts/addVersion.py{}}}) that never carries a qualifier. The
> Gradle build version, by contrast, defaults to {{-SNAPSHOT}}
> ({{{}build.gradle{}}}: {{baseVersion}} + {{{}version.suffix{}}}), and only an
> actual release build overrides it via {{{}-Dversion.release=x.y.z{}}}.
> So on every branch, between releases, the two disagree: the build produces
> {{10.1.0-SNAPSHOT}} while {{SolrVersion.LATEST}} claims to be {{{}10.1.0{}}}.
> Note this is not a main-vs-release-branch distinction – {{branch_10_0}} is
> equally {{10.0.1-SNAPSHOT}} between bugfix releases. Nothing enforces
> consistency between {{{}build.gradle{}}}'s {{baseVersion}} and
> {{{}LATEST_STRING{}}}.
> Arguably {{LATEST_STRING}} should carry the same {{-SNAPSHOT}} suffix the
> build does, so that a dev build does not misrepresent itself as a release.
> Before that can even be considered, however, {{SolrVersion}} and its callers
> have to tolerate a prerelease qualifier – and today they do not.
> h2. Problems
> Verified against semver4j 6.0.0 (the backing library). Parsing itself is
> fine: {{new Semver("10.1.0-SNAPSHOT")}} works and major/minor/patch are still
> 10/1/0. Build metadata ({{{}+b123{}}}) is harmless. Prerelease qualifiers are
> not.
> h3. 1. satisfies() rejects everything
> semver4j follows node-semver semantics, where a prerelease version satisfies
> no range unless the range itself names a prerelease:
> {code:java}
> new Semver("10.1.0-SNAPSHOT").satisfies(">=9.0.0") -> false
> .satisfies("10.x") -> false
> .satisfies("~10.1") -> false
> {code}
> {color:#ffab00}_(If you read that and aren't shocked; nothing can shock
> you!)_{color}
> {{PackageManager}} (~line 941) gates package installs on
> {{{}SolrVersion.LATEST.satisfies(manifest.versionConstraint){}}}. With a
> suffixed {{{}LATEST{}}}, *every* package would be rejected on a dev build.
> This is the hard blocker.
> That rule exists for npm dependency resolution, where you must not silently
> pull a prerelease. It is the wrong rule for "which Solr line is this node".
> h3. 2. Ordering makes a dev build look older than the release
> Per semver, {{{}10.1.0-SNAPSHOT < 10.1.0{}}}.
> {{ZkController.checkClusterVersionCompatibility}} compares
> {{SolrVersion.LATEST}} against the lowest version among live nodes and warns
> "our version is older than cluster version". A dev node joining a cluster of
> released nodes of the same x.y would trip that warning, even though it is in
> fact the newer code.
> Not fatal – the refuse-to-start guards compare major and minor as ints, which
> prerelease does not affect – but the warning is wrong and the reported
> "lowest version in cluster" from {{ZkStateReader.fetchLowestSolrVersion}}
> becomes misleading in a mixed dev/release cluster.
> h3. 3. getPrereleaseVersion() misused as an implementation version
> {{{}SystemInfoProvider.getLuceneInfo(){}}}:
> {code:java}
> info.solrImplVersion = implVersion == null ?
> SolrVersion.LATEST.getPrereleaseVersion() : implVersion;
> {code}
> This is an independent pre-existing bug – a wrong method pick introduced in
> SOLR-16458 (PR #4078), not something the suffix causes. Today it happens to
> return {{""}} because {{LATEST}} has no qualifier, so nobody noticed. With a
> suffix it would report {{"SNAPSHOT"}} as the Solr implementation version over
> the {{/api/node/system}} API. {{NodeSystemInfoProviderTest}} currently
> asserts this behaviour.
> h2. Suggested direction
> Fix the three breakages first; they are worth fixing on their own merits
> regardless of whether {{LATEST_STRING}} ever grows a suffix.
> * Add {{{}SolrVersion.withoutPrerelease(){}}}, backed by semver4j's
> {{{}withClearedPreRelease(){}}}.
> * Have {{satisfies()}} evaluate on the cleared version, documented as
> ignoring the qualifier. This fixes {{PackageManager}} and keeps future
> callers off the same landmine. (Alternative: leave {{satisfies()}} literal
> and strip at the {{PackageManager}} call site – more explicit, less
> protective.)
> * Leave {{{}compareTo{}}}/{{{}equals{}}} as strict semver; they are public
> API and {{-SNAPSHOT < release}} is correct there. Instead normalise at the
> one ordering site that cares, {{{}ZkController{}}}'s {{lessThan}} check,
> while still printing full versions in the log and exception messages.
> * Fix the {{solrImplVersion}} fallback to use {{{}LATEST_STRING{}}}, and
> update the test.
> Coverage note: there is no clean way to integration-test the {{ZkController}}
> fix, since tripping it requires *our own* version to be a prerelease and
> {{SolrVersion.LATEST}} is a compile-time constant with no injection seam.
> {{ZkControllerTest}} can only fabricate the cluster's version, which is the
> non-tripping direction. Coverage would land in {{TestSolrVersion}} unless we
> think a seam is worth adding. (DWS: not in favor of a seam)
> Whether {{LATEST_STRING}} should then actually carry {{{}-SNAPSHOT{}}}, and
> how the release process would strip it, is deliberately left to a follow-up.
> The likely cleanest answer is to stop hardcoding it and generate it from the
> Gradle version, since {{-Dversion.release}} already produces exactly the
> right value for a release build and would need no release-wizard changes at
> all.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
