Lucene - Core
  1. Lucene - Core
  2. LUCENE-5850

Constants#LUCENE_MAIN_VERSION can have broken values

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.3.1, 4.5.1
    • Fix Version/s: 4.10, 6.0
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Constants#LUCENE_MAIN_VERSION is set to the Lucene Main version and should not contain minor versions. Well this is at least what I thought and to my knowledge what the comments say too. Yet in for instance 4.3.1 and 4.5.1 we broke this such that the version from SegmentsInfo can not be parsed with Version#parseLeniently. IMO we should really add an assertion that this constant doesn't throw an error and / or make the smoketester catch this. to me this is actually a index BWC break. Note that 4.8.1 doesn't have this problem...

      1. LUCENE-5850_bomb.patch
        3 kB
        Robert Muir
      2. LUCENE-5850_hashcode.patch
        2 kB
        Simon Willnauer
      3. LUCENE-5850_smoketester.patch
        1 kB
        Robert Muir
      4. LUCENE-5850.patch
        97 kB
        Ryan Ernst
      5. LUCENE-5850.patch
        97 kB
        Ryan Ernst
      6. LUCENE-5850.patch
        96 kB
        Ryan Ernst
      7. LUCENE-5850.patch
        17 kB
        Uwe Schindler
      8. LUCENE-5850.patch
        16 kB
        Uwe Schindler
      9. LUCENE-5850.patch
        15 kB
        Uwe Schindler
      10. LUCENE-5850.patch
        14 kB
        Uwe Schindler
      11. LUCENE-5850.patch
        13 kB
        Uwe Schindler
      12. LUCENE-5850.patch
        2 kB
        Michael McCandless
      13. LUCENE-5860_hashcode.patch
        2 kB
        Simon Willnauer

        Issue Links

          Activity

          Hide
          Simon Willnauer added a comment -

          for reference here are the affected versions:

          maybe there are more While 4.8.1 looks good

          Show
          Simon Willnauer added a comment - for reference here are the affected versions: 4.5.1 4.3.1 4.2.1 4.0.0.2 maybe there are more While 4.8.1 looks good
          Hide
          Robert Muir added a comment -

          It is intentional that this isnt changed:

          We should never change index format with minor versions, so it should always be x.y or x.y.0.z for alpha/beta versions!
          
          Show
          Robert Muir added a comment - It is intentional that this isnt changed: We should never change index format with minor versions, so it should always be x.y or x.y.0.z for alpha/beta versions!
          Hide
          Robert Muir added a comment -

          The real bug here is not what simon describes... (although I think its confusing, and its extra confusing there are "two versions").

          It is not that 4.8.1 is good, actually 4.8.1 is the buggy one. Segments with 4.8.1 say they were written with "4.8", which is broken.

          Furthermore, code like Lucene45DocValuesProducer doing stuff like the below is also broken:

          Version.parseLeniently(state.segmentInfo.getVersion());
          

          The main problem is caused by the overengineering of this shit: two different version values, one of which is driven by a system property, and other confusion. Because of this its also not tested. I realize this system property shit is supposed to be there to support "strange" things like custom builds and maven snapshots, but its gotta die. Sorry, its broken for actual lucene releases!

          Show
          Robert Muir added a comment - The real bug here is not what simon describes... (although I think its confusing, and its extra confusing there are "two versions"). It is not that 4.8.1 is good, actually 4.8.1 is the buggy one. Segments with 4.8.1 say they were written with "4.8", which is broken. Furthermore, code like Lucene45DocValuesProducer doing stuff like the below is also broken: Version.parseLeniently(state.segmentInfo.getVersion()); The main problem is caused by the overengineering of this shit: two different version values, one of which is driven by a system property, and other confusion. Because of this its also not tested. I realize this system property shit is supposed to be there to support "strange" things like custom builds and maven snapshots, but its gotta die. Sorry, its broken for actual lucene releases!
          Hide
          Simon Willnauer added a comment -

          I agree with robert though. I should have named this issue differently. It confused the hell out of me since I would have expected this to be always 4.x or 4.x.y but for 4.8.1 this value is actually 4.8 and apparently it should have been 4.8.1? But then the comment is broken too. We should really just use Version.java and have constants there for all the release?

          Show
          Simon Willnauer added a comment - I agree with robert though. I should have named this issue differently. It confused the hell out of me since I would have expected this to be always 4.x or 4.x.y but for 4.8.1 this value is actually 4.8 and apparently it should have been 4.8.1? But then the comment is broken too. We should really just use Version.java and have constants there for all the release?
          Hide
          Uwe Schindler added a comment -

          Yeah, this is horrible. 4.8.1 is not broken, because I took care of that before release.

          Actually, if we parse this to Version constants, the Lucene Main Version constant in Constants.java should be of type Version. And Version should be able to serialize as String according to spec.

          I think in the affected versions, the RM used search replace in an inappropriate way...

          The Constants test case should validate the version string with a regex. I think I already added that, so it should not happen later.

          The sys prop hell is only partly related but is there to validate that the common-build version fits the constant. At runtime it never parses constants. Lucene never reads the sysprop at runtime.

          Uwe Schindler
          H.-H.-Meier-Allee 63, 28213 Bremen
          http://www.thetaphi.de

          Show
          Uwe Schindler added a comment - Yeah, this is horrible. 4.8.1 is not broken, because I took care of that before release. Actually, if we parse this to Version constants, the Lucene Main Version constant in Constants.java should be of type Version. And Version should be able to serialize as String according to spec. I think in the affected versions, the RM used search replace in an inappropriate way... The Constants test case should validate the version string with a regex. I think I already added that, so it should not happen later. The sys prop hell is only partly related but is there to validate that the common-build version fits the constant. At runtime it never parses constants. Lucene never reads the sysprop at runtime. – Uwe Schindler H.-H.-Meier-Allee 63, 28213 Bremen http://www.thetaphi.de
          Hide
          Robert Muir added a comment -

          Before doing anything else, we should commit this patch. Its a timebomb waiting to happen that these two places in the code do the wrong thing!

          Show
          Robert Muir added a comment - Before doing anything else, we should commit this patch. Its a timebomb waiting to happen that these two places in the code do the wrong thing!
          Hide
          ASF subversion and git services added a comment -

          Commit 1613988 from Robert Muir in branch 'dev/trunk'
          [ https://svn.apache.org/r1613988 ]

          LUCENE-5850: use correct version comparator (at the moment) for versions in index segments, otherwise this logic will do the wrong thing on the next bugfix release!

          Show
          ASF subversion and git services added a comment - Commit 1613988 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1613988 ] LUCENE-5850 : use correct version comparator (at the moment) for versions in index segments, otherwise this logic will do the wrong thing on the next bugfix release!
          Hide
          ASF subversion and git services added a comment -

          Commit 1614002 from Robert Muir in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1614002 ]

          LUCENE-5850: use correct version comparator (at the moment) for versions in index segments, otherwise this logic will do the wrong thing on the next bugfix release!

          Show
          ASF subversion and git services added a comment - Commit 1614002 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1614002 ] LUCENE-5850 : use correct version comparator (at the moment) for versions in index segments, otherwise this logic will do the wrong thing on the next bugfix release!
          Hide
          Robert Muir added a comment -

          Here is a start at some logic for the smoketester, after it runs the demo, it should run CheckIndex on the index. I modified CheckIndex to output the segment's version, and I think we can change this python script to verify its equal to the supplied version the smoketester is run with???

          Show
          Robert Muir added a comment - Here is a start at some logic for the smoketester, after it runs the demo, it should run CheckIndex on the index. I modified CheckIndex to output the segment's version, and I think we can change this python script to verify its equal to the supplied version the smoketester is run with???
          Hide
          Michael McCandless added a comment -

          Patch, with changes to smokeTestRelease to find the version in checkindex.log and see if it matches what was specified to its command-line. I strip trailing .0's like StringHelper's versionComparator. I ran it with "ant nightly-smoke" and it looks like it's gotten past this check successfully on 4.x ...

          Show
          Michael McCandless added a comment - Patch, with changes to smokeTestRelease to find the version in checkindex.log and see if it matches what was specified to its command-line. I strip trailing .0's like StringHelper's versionComparator. I ran it with "ant nightly-smoke" and it looks like it's gotten past this check successfully on 4.x ...
          Hide
          Robert Muir added a comment -

          Yes it should currently pass. But if you point it at release 4.8.1 artifacts it should fail?

          http://people.apache.org/~rmuir/staging_area/lucene_solr_4_8_1_r1594670/

          Show
          Robert Muir added a comment - Yes it should currently pass. But if you point it at release 4.8.1 artifacts it should fail? http://people.apache.org/~rmuir/staging_area/lucene_solr_4_8_1_r1594670/
          Hide
          Hoss Man added a comment -

          The main problem is caused by the overengineering of this shit: two different version values, one of which is driven by a system property, and other confusion. Because of this its also not tested. I realize this system property shit is supposed to be there to support "strange" things like custom builds and maven snapshots, but its gotta die. Sorry, its broken for actual lucene releases!

          Agreed: we should absolutely prioritize the stability and correctness of the official releases over any sort of custom builds and/or maven snapshots – but one thing to keep in mind is that a lot of the reason for these "defaults" was not to make it easier for people to do custom builds or maven snapshots, but to help ensure that if someone does a custom build or uses a maven snapshot there is no mistaking it from an official build.

          ie: way, way back in the day people would build off trunk, with custom patches and their builds might be named "lucene-1.4.jar" (before 1.4 was ever official) and then weeks/months later (after 1.4 came out) they (or their coworkers) reported bugs or asked questions that made no sense and there would be no end of confusion.

          Show
          Hoss Man added a comment - The main problem is caused by the overengineering of this shit: two different version values, one of which is driven by a system property, and other confusion. Because of this its also not tested. I realize this system property shit is supposed to be there to support "strange" things like custom builds and maven snapshots, but its gotta die. Sorry, its broken for actual lucene releases! Agreed: we should absolutely prioritize the stability and correctness of the official releases over any sort of custom builds and/or maven snapshots – but one thing to keep in mind is that a lot of the reason for these "defaults" was not to make it easier for people to do custom builds or maven snapshots, but to help ensure that if someone does a custom build or uses a maven snapshot there is no mistaking it from an official build. ie: way, way back in the day people would build off trunk, with custom patches and their builds might be named "lucene-1.4.jar" (before 1.4 was ever official) and then weeks/months later (after 1.4 came out) they (or their coworkers) reported bugs or asked questions that made no sense and there would be no end of confusion.
          Hide
          Michael McCandless added a comment -

          Well, in 4.8.1 CheckIndex doesn't print out SI's version, so smokeTestRelease.py gets upset:

          Traceback (most recent call last):
            File "dev-tools/scripts/smokeTestRelease.py", line 1360, in <module>
            File "dev-tools/scripts/smokeTestRelease.py", line 1304, in main
            File "dev-tools/scripts/smokeTestRelease.py", line 1341, in smokeTest
            File "dev-tools/scripts/smokeTestRelease.py", line 637, in unpackAndVerify
            File "dev-tools/scripts/smokeTestRelease.py", line 764, in verifyUnpacked
            File "dev-tools/scripts/smokeTestRelease.py", line 948, in testDemo
          RuntimeError: unable to locate version=NNN output from CheckIndex; see checkindex.log
          
          Show
          Michael McCandless added a comment - Well, in 4.8.1 CheckIndex doesn't print out SI's version, so smokeTestRelease.py gets upset: Traceback (most recent call last): File "dev-tools/scripts/smokeTestRelease.py", line 1360, in <module> File "dev-tools/scripts/smokeTestRelease.py", line 1304, in main File "dev-tools/scripts/smokeTestRelease.py", line 1341, in smokeTest File "dev-tools/scripts/smokeTestRelease.py", line 637, in unpackAndVerify File "dev-tools/scripts/smokeTestRelease.py", line 764, in verifyUnpacked File "dev-tools/scripts/smokeTestRelease.py", line 948, in testDemo RuntimeError: unable to locate version=NNN output from CheckIndex; see checkindex.log
          Hide
          Robert Muir added a comment -

          +1 to commit this to the smoke tester. I have the feeling that "total overhaul of versioning to work correctly" will be painful and controversial, but we can at least do this for now.

          Show
          Robert Muir added a comment - +1 to commit this to the smoke tester. I have the feeling that "total overhaul of versioning to work correctly" will be painful and controversial, but we can at least do this for now.
          Hide
          Robert Muir added a comment -

          Agreed: we should absolutely prioritize the stability and correctness of the official releases over any sort of custom builds and/or maven snapshots – but one thing to keep in mind is that a lot of the reason for these "defaults" was not to make it easier for people to do custom builds or maven snapshots, but to help ensure that if someone does a custom build or uses a maven snapshot there is no mistaking it from an official build.

          I don't really care about this. The stuff we record in the index needs to be correct for various back compat to work. If we can't simplify the versioning, then we need to simplify our back compat guarantee. But something has to give.

          Show
          Robert Muir added a comment - Agreed: we should absolutely prioritize the stability and correctness of the official releases over any sort of custom builds and/or maven snapshots – but one thing to keep in mind is that a lot of the reason for these "defaults" was not to make it easier for people to do custom builds or maven snapshots, but to help ensure that if someone does a custom build or uses a maven snapshot there is no mistaking it from an official build. I don't really care about this. The stuff we record in the index needs to be correct for various back compat to work. If we can't simplify the versioning, then we need to simplify our back compat guarantee. But something has to give.
          Hide
          ASF subversion and git services added a comment -

          Commit 1614136 from Michael McCandless in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1614136 ]

          LUCENE-5850: fix smoke tester to verify the index's segment versions in fact match the version being tested

          Show
          ASF subversion and git services added a comment - Commit 1614136 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1614136 ] LUCENE-5850 : fix smoke tester to verify the index's segment versions in fact match the version being tested
          Hide
          ASF subversion and git services added a comment -

          Commit 1614149 from Michael McCandless in branch 'dev/trunk'
          [ https://svn.apache.org/r1614149 ]

          LUCENE-5850: fix smoke tester to verify the index's segment versions in fact match the version being tested

          Show
          ASF subversion and git services added a comment - Commit 1614149 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1614149 ] LUCENE-5850 : fix smoke tester to verify the index's segment versions in fact match the version being tested
          Hide
          Uwe Schindler added a comment -

          Hi,

          thanks for all the work done already. I am on vacation at the moment, so sorry for not responding in time I was talking to Robert and Simon today via Google Hangouts (my internet was slow during the day). This issue contains several issues, which should be handled separately.

          In fact we have actually 2 problems:

          • The documentation of the LUCENE_MAIN_VERSION constant is inconsistent with its use. Originally this was meant to be the "dot version only", because we decided to never change the index format in bugfix versions. Because of this the suggested format was "x.y" (and the special case for alpha/beta introduced by Robert). Unfortunately in some releases, the RM just search/replaced all version numbers and we suddenly got minor versions there that were written into the index. This is not really bad at all, although its stupid for file format versions (because it did not change). I listened to Simon and Robert - both said that we should write the real version into the index file! We do this already, but only in the commit metadata, not per segment. We write the full version string there, including the RM's name and so on. So we record this metadata in the index. But we cannot get the minor version for each segment. So we have to decide if we write the bugfix version number to index file (x.y.z.a.b.c... as many as you like). If we do this, we need the validation of LUCENE_MAIN_VERSION (smoker and -> see below).
          • Because of the documentation and missing knowledge, Shai committed a change to DocValues that uses Version.parseLeniently() to check the index version. Shai already added some catch blocks to work around issues (he mentioned them in the comment....!!!), instead of using the officially written version comparator in StringUtils. This was a major bug, because in Version 4.9.1, the shit would be smoking I am glad that Robert fixed it! Many thanks, you are the best!!!

          The documentation of the Version class is perfectly fine, maybe we should add a not, that this class and parser is only for parsing versions that are user-input from config files. Also the Version class is only for passing a hint to the API, which behaviour you want to have. The Version class is not for file formats and should never serialized or used to switch inside codecs depending on file versions. We can fix two things:

          • Add StringUtils to Javadocs (@see with hint) of Version class
          • Maybe add a "hack to Version.parseLeniently to strip everything after the second dot (it's just a change of regex). By this users can enter 4.9.1, but it will return LUCENE_4_9.

          I am happy that Mike added a Smoke tester check, because the test in our code base was not good enough. But we should fix the version test correctly. Now I refer to Robert's complaint about the system property: The system property in the test runner was added to prevent the bug we have. This one passes the -Dversion=... constant down to tests, so we can validate LUCENE_MAIN_VERSION. Unfortunately the "version" sysprop also contains bullshit after the main version, so we just do a "startsWith" check in the test. And this is the bug. If LUCENE_MAIN_VERSION=="4.8" and we pass -Dversion=4.8.1-foobar the test passes, because the sysprop starts with the MAIN_VERSION.

          I think the perfect fix can be done since I fixed common-build to have the main version and the appendix separately. "version" is constrcuted in common-build from a prefix and suffix:

            <property name="dev.version.base" value="5.0"/>
            <property name="dev.version.suffix" value="SNAPSHOT"/>
            <property name="dev.version" value="${dev.version.base}-${dev.version.suffix}"/>
            <property name="version" value="${dev.version}"/>
          

          In fact we should change this and pass "dev.version.base" to the test runner and do an "equals" check in the testcase. By that we guarantee that common-build's version is identical to the LUCENE_MAIN_VERSION.

          Show
          Uwe Schindler added a comment - Hi, thanks for all the work done already. I am on vacation at the moment, so sorry for not responding in time I was talking to Robert and Simon today via Google Hangouts (my internet was slow during the day). This issue contains several issues, which should be handled separately. In fact we have actually 2 problems: The documentation of the LUCENE_MAIN_VERSION constant is inconsistent with its use. Originally this was meant to be the "dot version only", because we decided to never change the index format in bugfix versions. Because of this the suggested format was "x.y" (and the special case for alpha/beta introduced by Robert). Unfortunately in some releases, the RM just search/replaced all version numbers and we suddenly got minor versions there that were written into the index. This is not really bad at all, although its stupid for file format versions (because it did not change). I listened to Simon and Robert - both said that we should write the real version into the index file! We do this already, but only in the commit metadata, not per segment. We write the full version string there, including the RM's name and so on. So we record this metadata in the index. But we cannot get the minor version for each segment. So we have to decide if we write the bugfix version number to index file (x.y.z.a.b.c... as many as you like). If we do this, we need the validation of LUCENE_MAIN_VERSION (smoker and -> see below). Because of the documentation and missing knowledge, Shai committed a change to DocValues that uses Version.parseLeniently() to check the index version. Shai already added some catch blocks to work around issues (he mentioned them in the comment....!!!), instead of using the officially written version comparator in StringUtils. This was a major bug, because in Version 4.9.1, the shit would be smoking I am glad that Robert fixed it! Many thanks, you are the best!!! The documentation of the Version class is perfectly fine, maybe we should add a not, that this class and parser is only for parsing versions that are user-input from config files. Also the Version class is only for passing a hint to the API, which behaviour you want to have. The Version class is not for file formats and should never serialized or used to switch inside codecs depending on file versions. We can fix two things: Add StringUtils to Javadocs ( @see with hint) of Version class Maybe add a "hack to Version.parseLeniently to strip everything after the second dot (it's just a change of regex). By this users can enter 4.9.1, but it will return LUCENE_4_9. I am happy that Mike added a Smoke tester check, because the test in our code base was not good enough. But we should fix the version test correctly. Now I refer to Robert's complaint about the system property: The system property in the test runner was added to prevent the bug we have. This one passes the -Dversion=... constant down to tests, so we can validate LUCENE_MAIN_VERSION. Unfortunately the "version" sysprop also contains bullshit after the main version, so we just do a "startsWith" check in the test. And this is the bug. If LUCENE_MAIN_VERSION=="4.8" and we pass -Dversion=4.8.1-foobar the test passes, because the sysprop starts with the MAIN_VERSION. I think the perfect fix can be done since I fixed common-build to have the main version and the appendix separately. "version" is constrcuted in common-build from a prefix and suffix: <property name= "dev.version.base" value= "5.0" /> <property name= "dev.version.suffix" value= "SNAPSHOT" /> <property name= "dev.version" value= "${dev.version.base}-${dev.version.suffix}" /> <property name= "version" value= "${dev.version}" /> In fact we should change this and pass "dev.version.base" to the test runner and do an "equals" check in the testcase. By that we guarantee that common-build's version is identical to the LUCENE_MAIN_VERSION.
          Hide
          Robert Muir added a comment -

          But we cannot get the minor version for each segment. So we have to decide if we write the bugfix version number to index file (x.y.z.a.b.c... as many as you like). If we do this, we need the validation of LUCENE_MAIN_VERSION (smoker and -> see below).

          I think we should: because its the version of lucene that wrote the segment. It is possible we may need to differentiate 4.8.1 from 4.8 for some hackedy-hack in the future because of say, a bug unrelated to the format (perhaps in indexwriter or something). So if we write the correct version we always have the possibility to add workarounds for such things.

          Show
          Robert Muir added a comment - But we cannot get the minor version for each segment. So we have to decide if we write the bugfix version number to index file (x.y.z.a.b.c... as many as you like). If we do this, we need the validation of LUCENE_MAIN_VERSION (smoker and -> see below). I think we should: because its the version of lucene that wrote the segment. It is possible we may need to differentiate 4.8.1 from 4.8 for some hackedy-hack in the future because of say, a bug unrelated to the format (perhaps in indexwriter or something). So if we write the correct version we always have the possibility to add workarounds for such things.
          Hide
          Uwe Schindler added a comment -

          I think we should

          Then we have to change this code: After the problems with the minor versions appearing in LUCENE_MAIN_VERSION, this test was added - which enforces what the Javadocs about LUCENE_MAIN_VERSION tells:

            public void testLuceneMainVersionConstant() {
              assertTrue("LUCENE_MAIN_VERSION does not follow pattern: 'x.y' (stable release) or 'x.y.0.z' (alpha/beta version)" + getVersionDetails(),
                  Constants.LUCENE_MAIN_VERSION.matches("\\d+\\.\\d+(|\\.0\\.\\d+)"));
              assertTrue("LUCENE_VERSION does not start with LUCENE_MAIN_VERSION (without alpha/beta marker)" + getVersionDetails(),
                  Constants.LUCENE_VERSION.startsWith(Constants.mainVersionWithoutAlphaBeta()));
            }
          

          If we now use the full version number in the segmentinfos, we have to make LUCENE_MAIN_VERSION be identical to common-build's dev.version.base. I would then change this test to enforce identical values and remove the regexp and replace by a had check on the common-build sysprop in the test (see above). I can provide a patch doing this.

          Show
          Uwe Schindler added a comment - I think we should Then we have to change this code: After the problems with the minor versions appearing in LUCENE_MAIN_VERSION, this test was added - which enforces what the Javadocs about LUCENE_MAIN_VERSION tells: public void testLuceneMainVersionConstant() { assertTrue( "LUCENE_MAIN_VERSION does not follow pattern: 'x.y' (stable release) or 'x.y.0.z' (alpha/beta version)" + getVersionDetails(), Constants.LUCENE_MAIN_VERSION.matches( "\\d+\\.\\d+(|\\.0\\.\\d+)" )); assertTrue( "LUCENE_VERSION does not start with LUCENE_MAIN_VERSION (without alpha/beta marker)" + getVersionDetails(), Constants.LUCENE_VERSION.startsWith(Constants.mainVersionWithoutAlphaBeta())); } If we now use the full version number in the segmentinfos, we have to make LUCENE_MAIN_VERSION be identical to common-build's dev.version.base . I would then change this test to enforce identical values and remove the regexp and replace by a had check on the common-build sysprop in the test (see above). I can provide a patch doing this.
          Hide
          Robert Muir added a comment -

          Please do, afterwards we can then work on merging version string logic and Version.java

          We need only one version here. Currently the shit is overengineered and does not really work, even committers are screwing it up. To hell with maven builds, custom builds, etc, we have to fix this.

          Show
          Robert Muir added a comment - Please do, afterwards we can then work on merging version string logic and Version.java We need only one version here. Currently the shit is overengineered and does not really work, even committers are screwing it up. To hell with maven builds, custom builds, etc, we have to fix this.
          Hide
          Mark Miller added a comment -

          We need only one version here. Currently the shit is overengineered and does not really work, even committers are screwing it up. To hell with maven builds, custom builds, etc, we have to fix this

          I'm too high level on this to fully understand atm, so excuse my laziness, but can't we still basically support custom builds even with only one version? Can't they just override this one version?

          Even the ways things are now, we are working around issues with our custom build and replacing the version. I've had to comment out a few of those version check tests. I've talked about it with Robert before and it did all just seem to be kind of silly. Glad it's all getting overhauled.

          Just trying to understand why custom build would be too difficult anyway. I mean, perhaps you have to make a small hack to the ant file or something, but that's fine.

          Show
          Mark Miller added a comment - We need only one version here. Currently the shit is overengineered and does not really work, even committers are screwing it up. To hell with maven builds, custom builds, etc, we have to fix this I'm too high level on this to fully understand atm, so excuse my laziness, but can't we still basically support custom builds even with only one version? Can't they just override this one version? Even the ways things are now, we are working around issues with our custom build and replacing the version. I've had to comment out a few of those version check tests. I've talked about it with Robert before and it did all just seem to be kind of silly. Glad it's all getting overhauled. Just trying to understand why custom build would be too difficult anyway. I mean, perhaps you have to make a small hack to the ant file or something, but that's fine.
          Hide
          Robert Muir added a comment -

          Just trying to understand why custom build would be too difficult anyway. I mean, perhaps you have to make a small hack to the ant file or something, but that's fine.

          Its currently attempted to be supported and thats why versioning is a mess (there are multiple versions) and nobody understands what is going on. We have to just simplify it: its open source. If you want to do something custom, my god you might have to change the source

          Show
          Robert Muir added a comment - Just trying to understand why custom build would be too difficult anyway. I mean, perhaps you have to make a small hack to the ant file or something, but that's fine. Its currently attempted to be supported and thats why versioning is a mess (there are multiple versions) and nobody understands what is going on. We have to just simplify it: its open source. If you want to do something custom, my god you might have to change the source
          Hide
          Uwe Schindler added a comment - - edited

          There is nothing complicated anymore. As I said, I can fix the test. We already have splitted the common-build.xml version numbers in the base version and the appendix. The testcase just has to check if the base version is identical to the LUCENE_MAIN_VERSION constant. We are on the safe side then. The user can change the appendix as he likes (jenkins does this at the moment). The problem is just our test configuraton, I will provide a patch once I am back at home, just give me one or 2 days It is all supported, just the testcase is bullshit - and I will fix this.

          Show
          Uwe Schindler added a comment - - edited There is nothing complicated anymore. As I said, I can fix the test. We already have splitted the common-build.xml version numbers in the base version and the appendix. The testcase just has to check if the base version is identical to the LUCENE_MAIN_VERSION constant. We are on the safe side then. The user can change the appendix as he likes (jenkins does this at the moment). The problem is just our test configuraton, I will provide a patch once I am back at home, just give me one or 2 days It is all supported, just the testcase is bullshit - and I will fix this.
          Hide
          Robert Muir added a comment -

          There is nothing complicated anymore.

          I disagree, I see:

          • Constants.java:LUCENE_MAIN_VERSION
          • Constants.java:LUCENE_VERSION
          • Version.java
          • common-build.xml:dev-version.base
          • common-build.xml:dev-version.suffix
          • common-build.xml:dev-version
          • common-build.xml:version
          • common-build.xml:spec.version
          • common.build.xml:tests.luceneMatchVersion

          To me this is COMPLICATED!

          Show
          Robert Muir added a comment - There is nothing complicated anymore. I disagree, I see: Constants.java:LUCENE_MAIN_VERSION Constants.java:LUCENE_VERSION Version.java common-build.xml:dev-version.base common-build.xml:dev-version.suffix common-build.xml:dev-version common-build.xml:version common-build.xml:spec.version common.build.xml:tests.luceneMatchVersion To me this is COMPLICATED!
          Hide
          Uwe Schindler added a comment -

          To me this is COMPLICATED!

          I was talking about the test. Please let me fix it tomorrow, I just have no time at the moment. spec.version is already obslete its just a relict. The stuff with suffix and base was introduced to make handling the version easier, it was just not used to unfuck the test.

          Show
          Uwe Schindler added a comment - To me this is COMPLICATED! I was talking about the test. Please let me fix it tomorrow, I just have no time at the moment. spec.version is already obslete its just a relict. The stuff with suffix and base was introduced to make handling the version easier, it was just not used to unfuck the test.
          Hide
          Uwe Schindler added a comment -

          dev-version and version are also duplicates. I had this on the agenda to fix.

          Show
          Uwe Schindler added a comment - dev-version and version are also duplicates. I had this on the agenda to fix.
          Hide
          Uwe Schindler added a comment - - edited

          Hi,

          Please note: The problem mentioned in the original issue description ("Constants#LUCENE_MAIN_VERSION can have broken values") was not a problem anymore because of LUCENE-5537 (since 4.8). The original documentation said that LUCENE_MAIN_VERSION should only be in format x.y but because of the missing test introduced in LUCENE-5537, we had some outlyers in the past. 4.8.1 was therefore correct with LUCENE_MAIN_VERSION="4.8".

          In this issue we now decided that we should record the full (numeric) version number (without appendinxes) in index segments. Smoke tester already checks this now, but the tests were not confirming (still enforcing x.y format from LUCENE-5537).

          I worked on a patch that cleans up the version handling in Constants.java/Version.java and common-build.xml. It also contains improved tests:

          • LUCENE_MAIN_VERSION must be identical to common-builds "version.base" property. This is checked in the test. Previously, this was just a broken startsWith check on the full version.
          • I cleaned up all broken constants in common-build. We now solely have "version.base" and "version.suffix". "version" is constructed from both. The RM can enforce a release version without appendix by passing -Dversion=..., but common-build.xml validates the value passed in conformance with the fixed version.base constant, so you cannot release 4.10 with wrong MAIN_VERSION that would affect constants written to segments.
          • To support ALPHA/BETA versions (like Robert introduced in 4.0-ALPHA), you can set a boolean to true in Constants.java. By that it automatically appends ".0.1" at the end of the MAIN_VERSION. In my opinion, we should rethink this, the idea was cool, but adds stupid complexity to the simple checks and changes I did. Maybe we should release ALPHA/BETA versions in a different way with different versioning). For now I implemented it like Robert did it. Maybe Roert can review, I am not really happy.
          • I changed Version.parseLeniently() to allow minor bugfix versions given (so users can write "4.9.1" into their config files, but its parsed to LUCENE_4_9), which would be the corresponding match version constant. This is also checked in the test. This also solves the 1.3.1 hotfix release of Elasticsearch, which also misused parseLeniently in an non-approp way (like Shai did): http://github.com/elasticsearch/elasticsearch/issues/issue/7055
          Show
          Uwe Schindler added a comment - - edited Hi, Please note: The problem mentioned in the original issue description ("Constants#LUCENE_MAIN_VERSION can have broken values") was not a problem anymore because of LUCENE-5537 (since 4.8). The original documentation said that LUCENE_MAIN_VERSION should only be in format x.y but because of the missing test introduced in LUCENE-5537 , we had some outlyers in the past. 4.8.1 was therefore correct with LUCENE_MAIN_VERSION="4.8". In this issue we now decided that we should record the full (numeric) version number (without appendinxes) in index segments. Smoke tester already checks this now, but the tests were not confirming (still enforcing x.y format from LUCENE-5537 ). I worked on a patch that cleans up the version handling in Constants.java/Version.java and common-build.xml. It also contains improved tests: LUCENE_MAIN_VERSION must be identical to common-builds "version.base" property. This is checked in the test. Previously, this was just a broken startsWith check on the full version. I cleaned up all broken constants in common-build. We now solely have "version.base" and "version.suffix". "version" is constructed from both. The RM can enforce a release version without appendix by passing -Dversion=... , but common-build.xml validates the value passed in conformance with the fixed version.base constant, so you cannot release 4.10 with wrong MAIN_VERSION that would affect constants written to segments. To support ALPHA/BETA versions (like Robert introduced in 4.0-ALPHA), you can set a boolean to true in Constants.java. By that it automatically appends ".0.1" at the end of the MAIN_VERSION. In my opinion, we should rethink this, the idea was cool, but adds stupid complexity to the simple checks and changes I did. Maybe we should release ALPHA/BETA versions in a different way with different versioning). For now I implemented it like Robert did it. Maybe Roert can review, I am not really happy. I changed Version.parseLeniently() to allow minor bugfix versions given (so users can write "4.9.1" into their config files, but its parsed to LUCENE_4_9), which would be the corresponding match version constant. This is also checked in the test. This also solves the 1.3.1 hotfix release of Elasticsearch, which also misused parseLeniently in an non-approp way (like Shai did): http://github.com/elasticsearch/elasticsearch/issues/issue/7055
          Hide
          Robert Muir added a comment -

          To support ALPHA/BETA versions (like Robert introduced in 4.0-ALPHA), you can set a boolean to true in Constants.java. By that it automatically appends ".0.1" at the end of the MAIN_VERSION.

          But thats not what we did. Alpha, Beta, and Final all had 3 distinct version numbers.

          I changed Version.parseLeniently() to allow minor bugfix versions given (so users can write "4.9.1" into their config files, but its parsed to LUCENE_4_9), which would be the corresponding match version constant

          Why not just merge with Version.java to eliminate yet-another-version? We still have too many, and this is still asking for bugs, just different types of bugs. We can just add the previous release constants to the enum.

          Show
          Robert Muir added a comment - To support ALPHA/BETA versions (like Robert introduced in 4.0-ALPHA), you can set a boolean to true in Constants.java. By that it automatically appends ".0.1" at the end of the MAIN_VERSION. But thats not what we did. Alpha, Beta, and Final all had 3 distinct version numbers. I changed Version.parseLeniently() to allow minor bugfix versions given (so users can write "4.9.1" into their config files, but its parsed to LUCENE_4_9), which would be the corresponding match version constant Why not just merge with Version.java to eliminate yet-another-version? We still have too many, and this is still asking for bugs, just different types of bugs. We can just add the previous release constants to the enum.
          Hide
          Uwe Schindler added a comment - - edited

          But thats not what we did. Alpha, Beta, and Final all had 3 distinct version numbers.

          I know, this was just a quick hack to support maybe the same in Lucene 5.0. The idea of the ".0.1" was just to prevent people from using indexes created in beta versions with the released version. So the ".0.1" was perfectly fine to enforce this. But it did not help once 4.1 was out. ndexes created with 4.0.ALPHA did not fail if read with 4.1 (only with 4.0)! In my opinion, we should mark BETA indexes in a different way with LUCENE 5.0. This is why I said this should be reviewed. We should have a marker "ALPHA/BETA" in the index file and fail to read it, This is unrelated to this patch - I would be happy to remove the whole ALPHA/BETA checks. To emulate your behaviour, we could remove

          Why not just merge with Version.java to eliminate yet-another-version? We still have too many, and this is still asking for bugs, just different types of bugs. We can just add the previous release constants to the enum.

          I disagree. Version.java was and is never intended to be used to specifiy real Lucene versions. Version is there to enable backwards compatibility hacks when passing in as constant to enable backwards compatibility hacks. We also remove older constants!

          Elasticsearch and Shai Erea were simply doing the completely wrong thing. Version constants are there to enable backwards layers. parseLeniently was there to allow users to specify. Why does Elasticsearch not simply print the fucking String from the segment metadata? Sorry, what is described in the issue and the fix in ES 1.3.1 is bullshit^3!

          The parseLeniently hack added here was just for convenience. I should remove it completely - it was just here to help Elasticsearch! Sorry - will go away in next patch.

          In Lucene we just have 2 versions: Constants for reporting metadata and save it in index files. The Version enum is for enabling backwards hacks in Solr's and ES's config files. The parser was added to help Lucene users to pass those constants in config files.

          The big question to Simon Willnauer: Why does ES not report the real version number as string in the welcome message of its root rest endpoint???

          Show
          Uwe Schindler added a comment - - edited But thats not what we did. Alpha, Beta, and Final all had 3 distinct version numbers. I know, this was just a quick hack to support maybe the same in Lucene 5.0. The idea of the ".0.1" was just to prevent people from using indexes created in beta versions with the released version. So the ".0.1" was perfectly fine to enforce this. But it did not help once 4.1 was out. ndexes created with 4.0.ALPHA did not fail if read with 4.1 (only with 4.0)! In my opinion, we should mark BETA indexes in a different way with LUCENE 5.0. This is why I said this should be reviewed. We should have a marker "ALPHA/BETA" in the index file and fail to read it, This is unrelated to this patch - I would be happy to remove the whole ALPHA/BETA checks. To emulate your behaviour, we could remove Why not just merge with Version.java to eliminate yet-another-version? We still have too many, and this is still asking for bugs, just different types of bugs. We can just add the previous release constants to the enum. I disagree. Version.java was and is never intended to be used to specifiy real Lucene versions. Version is there to enable backwards compatibility hacks when passing in as constant to enable backwards compatibility hacks. We also remove older constants! Elasticsearch and Shai Erea were simply doing the completely wrong thing. Version constants are there to enable backwards layers. parseLeniently was there to allow users to specify. Why does Elasticsearch not simply print the fucking String from the segment metadata? Sorry, what is described in the issue and the fix in ES 1.3.1 is bullshit^3! The parseLeniently hack added here was just for convenience. I should remove it completely - it was just here to help Elasticsearch! Sorry - will go away in next patch. In Lucene we just have 2 versions: Constants for reporting metadata and save it in index files. The Version enum is for enabling backwards hacks in Solr's and ES's config files. The parser was added to help Lucene users to pass those constants in config files. The big question to Simon Willnauer : Why does ES not report the real version number as string in the welcome message of its root rest endpoint???
          Hide
          Robert Muir added a comment -

          I disagree. Version.java was and is never intended to be used to specifiy real Lucene versions. Version is there to enable backwards compatibility hacks when passing in as constant to enable backwards compatibility hacks. We also remove older constants!

          Elasticsearch and Shai Erea were simply doing the completely wrong thing...

          Of course, but the version "system" is so complicated that nobody here can even seem to agree about what should be written in the index.

          This is a big fucking problem.

          I'm telling you one thing: we are going to simplify this versioning, or we are going to simplify (aka reduce) our back compat guarantee. I'll be the first to opt out of providing back compat: its a feature that people who care about can contribute.

          I'm ready to go this route if you cannot simply agree that this is all a confusing mess.

          Show
          Robert Muir added a comment - I disagree. Version.java was and is never intended to be used to specifiy real Lucene versions. Version is there to enable backwards compatibility hacks when passing in as constant to enable backwards compatibility hacks. We also remove older constants! Elasticsearch and Shai Erea were simply doing the completely wrong thing... Of course, but the version "system" is so complicated that nobody here can even seem to agree about what should be written in the index. This is a big fucking problem. I'm telling you one thing: we are going to simplify this versioning, or we are going to simplify (aka reduce) our back compat guarantee. I'll be the first to opt out of providing back compat: its a feature that people who care about can contribute. I'm ready to go this route if you cannot simply agree that this is all a confusing mess.
          Hide
          Uwe Schindler added a comment -

          Can we move the ranting here to a more funny fight on the mailing list?

          Show
          Uwe Schindler added a comment - Can we move the ranting here to a more funny fight on the mailing list?
          Hide
          Robert Muir added a comment -

          I think its extremely relevant to the issue, just look at the discussion above.

          Various confusion about whether "4.8" or "4.8.1" should be written into the index for a bugfix release, buggy usage of version API #9 instead of version API #8, etc.

          Its a real problem that I think we should address.

          Show
          Robert Muir added a comment - I think its extremely relevant to the issue, just look at the discussion above. Various confusion about whether "4.8" or "4.8.1" should be written into the index for a bugfix release, buggy usage of version API #9 instead of version API #8, etc. Its a real problem that I think we should address.
          Hide
          Uwe Schindler added a comment -

          Robert, I fully agree! - this is why we opened and resolved issue LUCENE-5537. We just had a few broken releases with respect to this.

          We should now clean up our version handling, do we agree on this? This is what I tried to do in this release.

          Show
          Uwe Schindler added a comment - Robert, I fully agree! - this is why we opened and resolved issue LUCENE-5537 . We just had a few broken releases with respect to this. We should now clean up our version handling, do we agree on this? This is what I tried to do in this release.
          Hide
          Robert Muir added a comment -

          We do agree on that. We also need to simplify it though. Having multiple ways to compare versions (e.g. Version.compare vs StringHelper.versionComparator) etc is really bad.

          I think its equally bad to "downgrade" 4.8.1 to 4.8 because we have two apis (versus having one unified enum api). Its still buggy if you do the wrong thing, so if previous mistakes are repeated again, like the ones that motivated this issue, it will still be equally buggy, just buggy in a different way.

          Show
          Robert Muir added a comment - We do agree on that. We also need to simplify it though. Having multiple ways to compare versions (e.g. Version.compare vs StringHelper.versionComparator) etc is really bad. I think its equally bad to "downgrade" 4.8.1 to 4.8 because we have two apis (versus having one unified enum api). Its still buggy if you do the wrong thing, so if previous mistakes are repeated again, like the ones that motivated this issue, it will still be equally buggy, just buggy in a different way.
          Hide
          Uwe Schindler added a comment -

          OK, so in my opinion we should resolve this issue, because it is a duplicate of LUCENE-5537. The problem Simon Willnauer complained about was already fixed there. We should create a new issues about:

          • Should we write full bugfix version to index, not just "x.y" and "x.y.0.z"? (the attached patch handles this, I would just revert VersionparserLeniently()) -> we agreed on that, but the issue should be committed in a new issue.
          • merge all Version comparators and solely use Version enum? Maybe replace Version enum by something else (hierarchical?)
          Show
          Uwe Schindler added a comment - OK, so in my opinion we should resolve this issue, because it is a duplicate of LUCENE-5537 . The problem Simon Willnauer complained about was already fixed there. We should create a new issues about: Should we write full bugfix version to index, not just "x.y" and "x.y.0.z"? (the attached patch handles this, I would just revert VersionparserLeniently()) -> we agreed on that, but the issue should be committed in a new issue. merge all Version comparators and solely use Version enum? Maybe replace Version enum by something else (hierarchical?)
          Hide
          Robert Muir added a comment -

          I don't think the real problem (confusion) is fixed yet. Seems to me that making a new issue will just make it harder to follow the discussion.

          Show
          Robert Muir added a comment - I don't think the real problem (confusion) is fixed yet. Seems to me that making a new issue will just make it harder to follow the discussion.
          Hide
          Uwe Schindler added a comment - - edited

          Then we should change issue title and description!

          Constants#LUCENE_MAIN_VERSION is set to the Lucene Main version and should not contain minor versions. Well this is at least what I thought and to my knowledge what the comments say too. Yet in for instance 4.3.1 and 4.5.1 we broke this such that the version from SegmentsInfo can not be parsed with Version#parseLeniently. IMO we should really add an assertion that this constant doesn't throw an error and / or make the smoketester catch this. to me this is actually a index BWC break. Note that 4.8.1 doesn't have this problem...

          This was fixed in LUCENE-5537 because it enforced LUCENE_MAIN_VERSION to be "x.y" or "x.y.0.z" via regex. The reason why Simon Willnauer opened the issue was, because he was hurt by the broken releases before the added check.

          The new issue is about the above 2 things:

          • recording bugfix version in segmentinfos
          • refactor Version
          Show
          Uwe Schindler added a comment - - edited Then we should change issue title and description! Constants#LUCENE_MAIN_VERSION is set to the Lucene Main version and should not contain minor versions. Well this is at least what I thought and to my knowledge what the comments say too. Yet in for instance 4.3.1 and 4.5.1 we broke this such that the version from SegmentsInfo can not be parsed with Version#parseLeniently. IMO we should really add an assertion that this constant doesn't throw an error and / or make the smoketester catch this. to me this is actually a index BWC break. Note that 4.8.1 doesn't have this problem... This was fixed in LUCENE-5537 because it enforced LUCENE_MAIN_VERSION to be "x.y" or "x.y.0.z" via regex. The reason why Simon Willnauer opened the issue was, because he was hurt by the broken releases before the added check. The new issue is about the above 2 things: recording bugfix version in segmentinfos refactor Version
          Hide
          Robert Muir added a comment -

          Right, to be clear, here we have calmed the symptoms (the back compat time-bomb bugs in SegmentReader and codec).
          But to me this isn't enough, i am afraid of what would happen if we released such code in a bugfix release.
          We need to cure the disease, and prevent such bugs from being easy to do.

          Additionally we should probably change the release instructions to generate back compat index for every release bugfix or not, index format change or not, and retroactively generate them for all 4.x releases. This would provide additional protection.

          Show
          Robert Muir added a comment - Right, to be clear, here we have calmed the symptoms (the back compat time-bomb bugs in SegmentReader and codec). But to me this isn't enough, i am afraid of what would happen if we released such code in a bugfix release. We need to cure the disease, and prevent such bugs from being easy to do. Additionally we should probably change the release instructions to generate back compat index for every release bugfix or not, index format change or not, and retroactively generate them for all 4.x releases. This would provide additional protection.
          Hide
          Uwe Schindler added a comment -

          I have seen, you are going to remove Version.java completely in LUCENE-5859, so I will not touch Version in this issue.

          I will rewrite the patch, to clean up common-build.xml and use the full bugfix version when writing index files.

          Show
          Uwe Schindler added a comment - I have seen, you are going to remove Version.java completely in LUCENE-5859 , so I will not touch Version in this issue. I will rewrite the patch, to clean up common-build.xml and use the full bugfix version when writing index files.
          Hide
          Robert Muir added a comment -

          I am happy also if Version is the "one thing" with the comparator and used for the index... (with all constants for all releases).

          And not mandatory for analyzers.

          Show
          Robert Muir added a comment - I am happy also if Version is the "one thing" with the comparator and used for the index... (with all constants for all releases). And not mandatory for analyzers.
          Hide
          Uwe Schindler added a comment -

          New patch:

          • I improved documentation of Version.java to prevent misuse like done in Elasticsearch or by Shai Erera.
          • Fixed Alpha/Beta handling
          • Point the Release manager exactly at the place where to change versions and document what he needs to do

          I think this is ready as a first step. Cleaning up "Version" class (removal) can be done in the LUCENE-5859.

          Show
          Uwe Schindler added a comment - New patch: I improved documentation of Version.java to prevent misuse like done in Elasticsearch or by Shai Erera . Fixed Alpha/Beta handling Point the Release manager exactly at the place where to change versions and document what he needs to do I think this is ready as a first step. Cleaning up "Version" class (removal) can be done in the LUCENE-5859 .
          Hide
          Uwe Schindler added a comment -

          Improved the common-build.xml checks. Also cleraly state where the RM has to take action!

          Show
          Uwe Schindler added a comment - Improved the common-build.xml checks. Also cleraly state where the RM has to take action!
          Hide
          Uwe Schindler added a comment -

          Small updates after chat with Robert: Improve documentation

          There is a small change now for release managers:
          As we now write the full version number into index files (including bugfix), after branching a release we have to update common-build.xml and Constants.java to have the bugfix number already. This is needed to have LUCENE_MAIN_VERSION already contain the bugfix number. This is checked by the common-build file. So you cannot release "4.10.0" without editing common-build.xml to be valid. The only difference between releases and other builds is the version suffix "SNAPSHOT".

          I will update the release manager instructions after this is done.

          For Robert: THIS PATCH DOES NOT HANDLE Version.java MERGE. IT JUST UPDATES TO NEW LUCENE_MAIN_VERSION_SEMANTICS (full bugfix) and improves documentation, so problems like ES and Shai's error cannot happen anymore.

          Show
          Uwe Schindler added a comment - Small updates after chat with Robert: Improve documentation There is a small change now for release managers: As we now write the full version number into index files (including bugfix), after branching a release we have to update common-build.xml and Constants.java to have the bugfix number already. This is needed to have LUCENE_MAIN_VERSION already contain the bugfix number. This is checked by the common-build file. So you cannot release "4.10.0" without editing common-build.xml to be valid. The only difference between releases and other builds is the version suffix "SNAPSHOT". I will update the release manager instructions after this is done. For Robert: THIS PATCH DOES NOT HANDLE Version.java MERGE. IT JUST UPDATES TO NEW LUCENE_MAIN_VERSION_SEMANTICS (full bugfix) and improves documentation, so problems like ES and Shai's error cannot happen anymore.
          Hide
          Shai Erera added a comment -

          Thanks for fixing this bug Rob ... "sorry" is really all I can say. I hope I didn't cause all this shit... Maybe we should also document SegmentInfos.getVersion() to use StringHelper if you want to parse it?

          Show
          Shai Erera added a comment - Thanks for fixing this bug Rob ... "sorry" is really all I can say. I hope I didn't cause all this shit... Maybe we should also document SegmentInfos.getVersion() to use StringHelper if you want to parse it?
          Hide
          Robert Muir added a comment -

          Why should you feel sorry? I dont think its your fault.

          I blame the overly complicated versioning, thats why i was arguing to simplify it.

          If you have N version strings and M comparators and each comparator only works correctly on a subset of the version strings (on the others, it gives exceptions or wrong answers), then its just asking for bugs.

          Show
          Robert Muir added a comment - Why should you feel sorry? I dont think its your fault. I blame the overly complicated versioning, thats why i was arguing to simplify it. If you have N version strings and M comparators and each comparator only works correctly on a subset of the version strings (on the others, it gives exceptions or wrong answers), then its just asking for bugs.
          Hide
          Uwe Schindler added a comment -

          Patch with Shai's suggestion.

          Show
          Uwe Schindler added a comment - Patch with Shai's suggestion.
          Hide
          Shai Erera added a comment -

          Thanks Uwe for adding the docs, they are crystal clear.

          Show
          Shai Erera added a comment - Thanks Uwe for adding the docs, they are crystal clear.
          Hide
          Uwe Schindler added a comment -

          How should we proceed with this.

          I would suggest to commit this for now, so we can then work on extending Version.java? This patch mainly restores Lucene 4.x and trunk into a releaseable state. The current one, would not work for minor version releases, because the tests would prevent this from working (the current code in trunk/4.x does not allow minor versions in LUCENE_MAIN_VERSION).

          So should I commit this to 4.x and trunk

          BTW: Lucene 4.9.1 would be releaseable, as the tests are enforcing LUCENE_MAIN_VERSION to be "4.9", not "4.9.1", so the problem Simon mentioned is not there. The additional smoketester check was not committed to 4.9 branch, so this would not break 4.9.1 release.

          Show
          Uwe Schindler added a comment - How should we proceed with this. I would suggest to commit this for now, so we can then work on extending Version.java? This patch mainly restores Lucene 4.x and trunk into a releaseable state. The current one, would not work for minor version releases, because the tests would prevent this from working (the current code in trunk/4.x does not allow minor versions in LUCENE_MAIN_VERSION). So should I commit this to 4.x and trunk BTW: Lucene 4.9.1 would be releaseable, as the tests are enforcing LUCENE_MAIN_VERSION to be "4.9", not "4.9.1", so the problem Simon mentioned is not there. The additional smoketester check was not committed to 4.9 branch, so this would not break 4.9.1 release.
          Hide
          Robert Muir added a comment -

          4.9.1 is not releasable. This issue needs to be fixed first to make it so (and several things backported, like fixes to SegmentReader).

          Show
          Robert Muir added a comment - 4.9.1 is not releasable. This issue needs to be fixed first to make it so (and several things backported, like fixes to SegmentReader).
          Hide
          Uwe Schindler added a comment - - edited

          Sorry Robert it is. In 4.9.1 it would write LUCENE_MAIN_VERSION with "4.9". My earlier fix enforces this. If you would use LUCENE_MAIN_VERSION="4.9.1" it would fail to run tests already.

          The bugs were in older versions that has an incorrect value. The bugs are indeed older 4.5.1 and 4.3.1 versions. See Simon's original issue description.

          The assumption you have is that it might happen that LUCENE_MAIN_VERSION is "4.9.1", which it can never be, unless you disable TestConstants, added in 4.7.

          Show
          Uwe Schindler added a comment - - edited Sorry Robert it is. In 4.9.1 it would write LUCENE_MAIN_VERSION with "4.9". My earlier fix enforces this. If you would use LUCENE_MAIN_VERSION="4.9.1" it would fail to run tests already. The bugs were in older versions that has an incorrect value. The bugs are indeed older 4.5.1 and 4.3.1 versions. See Simon's original issue description. The assumption you have is that it might happen that LUCENE_MAIN_VERSION is "4.9.1", which it can never be, unless you disable TestConstants, added in 4.7.
          Hide
          Robert Muir added a comment -

          I think this issue should definitely block any 4.9.1, sorry.

          This versioning stuff is a mess and I would vote -1 to any release without it fixed, otherwise the possibility of bugs and mistakes is just too high.

          Show
          Robert Muir added a comment - I think this issue should definitely block any 4.9.1, sorry. This versioning stuff is a mess and I would vote -1 to any release without it fixed, otherwise the possibility of bugs and mistakes is just too high.
          Hide
          Robert Muir added a comment -

          The assumption you have is that it might happen that LUCENE_MAIN_VERSION is "4.9.1", which it can never be

          But it should be if 4.9.1 is going to have any significant changes. Otherwise if we make a mistake, we cant correct it, as we will be unable to differentiate between 4.9 and 4.9.1 indexes.

          This is a really big problem.

          Show
          Robert Muir added a comment - The assumption you have is that it might happen that LUCENE_MAIN_VERSION is "4.9.1", which it can never be But it should be if 4.9.1 is going to have any significant changes. Otherwise if we make a mistake, we cant correct it, as we will be unable to differentiate between 4.9 and 4.9.1 indexes. This is a really big problem.
          Hide
          Uwe Schindler added a comment -

          But it should be if 4.9.1 is going to have any significant changes. Otherwise if we make a mistake, we cant correct it, as we will be unable to differentiate between 4.9 and 4.9.1 indexes.

          I agree, this is why we decided to do it differently. But this would not prevent a bugfix release of current 4.9 branch. It would just use the old semantics to only write the major x.y version to the index files.

          I 100% agree, we should fix this in 4.10!

          My question on this issue was just: Should I commit the current work and we then open a new issue to merge MAIN_VERSION and Version.java, as Ryan suggests? This issue has many cleanups and better tests, so its easier as base for the big "Version" refactoring by Ryan.

          Show
          Uwe Schindler added a comment - But it should be if 4.9.1 is going to have any significant changes. Otherwise if we make a mistake, we cant correct it, as we will be unable to differentiate between 4.9 and 4.9.1 indexes. I agree, this is why we decided to do it differently. But this would not prevent a bugfix release of current 4.9 branch. It would just use the old semantics to only write the major x.y version to the index files. I 100% agree, we should fix this in 4.10! My question on this issue was just: Should I commit the current work and we then open a new issue to merge MAIN_VERSION and Version.java, as Ryan suggests? This issue has many cleanups and better tests, so its easier as base for the big "Version" refactoring by Ryan.
          Hide
          Robert Muir added a comment -

          But this would not prevent a bugfix release of current 4.9 branch.

          Nope, it wouldnt. it would just prevent index backwards compatibility support for such a release. If we want to relax our index back compat requirement such that we don't need to be able to read such 4.9/4.9.1 in the future, then the versioning can be a mess.

          Otherwise it needs to be correct.

          Show
          Robert Muir added a comment - But this would not prevent a bugfix release of current 4.9 branch. Nope, it wouldnt. it would just prevent index backwards compatibility support for such a release. If we want to relax our index back compat requirement such that we don't need to be able to read such 4.9/4.9.1 in the future, then the versioning can be a mess. Otherwise it needs to be correct.
          Hide
          Ryan Ernst added a comment -

          This patch consolidates LUCENE_MAIN_VERSION and associated parsing/comparison code into Version.java. Specifically, it:

          • Deprecates Constants.LUCENE_MAIN_VERSION and Constants.LUCENE_VERSION
          • Makes Version a class instead of an enum. This allows forward compatibility when parsing (e.g. 4.10.0 being able to read a 3.6.5 index that is as yet unreleased)
          • Adds constants for specific releases (e.g. LUCENE_4_10_0) and deprecates older minor release constants (e.g. LUCENE_4_9)
          • Renames LUCENE_CURRENT constant (but with deprecation for backcompat) to LATEST, and removes deprecation. This now is a true alias for the latest version, and having latest deprecated, but not the actual latest version deprecated, was confusing.
          • Changes all uses of StringHelper.getVersionComparator() to use Version.onOrAfter
          • Makes SegmentInfo take a Version, instead of string
          • Removes the "display" version (replaced with toString() of the latest version). This didn't seem useful as it doesn't contain any interesting information, and would only contain extra information if built with svn (AFAICT)
          • Adds Version.parse() which only parses dot based versions. In general, I think everything should use this function and we should deprecate parseLeniently, but I've left the latter for now.
          • Removes snapshot logic as far as the version is concerned in code (it was only used for the display version)

          I've probably forgot some things. All tests pass.

          Show
          Ryan Ernst added a comment - This patch consolidates LUCENE_MAIN_VERSION and associated parsing/comparison code into Version.java. Specifically, it: Deprecates Constants.LUCENE_MAIN_VERSION and Constants.LUCENE_VERSION Makes Version a class instead of an enum. This allows forward compatibility when parsing (e.g. 4.10.0 being able to read a 3.6.5 index that is as yet unreleased) Adds constants for specific releases (e.g. LUCENE_4_10_0 ) and deprecates older minor release constants (e.g. LUCENE_4_9 ) Renames LUCENE_CURRENT constant (but with deprecation for backcompat) to LATEST , and removes deprecation. This now is a true alias for the latest version, and having latest deprecated, but not the actual latest version deprecated, was confusing. Changes all uses of StringHelper.getVersionComparator() to use Version.onOrAfter Makes SegmentInfo take a Version , instead of string Removes the "display" version (replaced with toString() of the latest version). This didn't seem useful as it doesn't contain any interesting information, and would only contain extra information if built with svn (AFAICT) Adds Version.parse() which only parses dot based versions. In general, I think everything should use this function and we should deprecate parseLeniently, but I've left the latter for now. Removes snapshot logic as far as the version is concerned in code (it was only used for the display version) I've probably forgot some things. All tests pass.
          Hide
          Ryan Ernst added a comment -

          I would like to commit this Friday morning PDT if there are no objections.

          Show
          Ryan Ernst added a comment - I would like to commit this Friday morning PDT if there are no objections.
          Hide
          Uwe Schindler added a comment -

          You patch no longer contains a check that the actual Version.LATEST is the one thats also in common-build.xml? I added that for extra safety.

          Show
          Uwe Schindler added a comment - You patch no longer contains a check that the actual Version.LATEST is the one thats also in common-build.xml? I added that for extra safety.
          Hide
          Ryan Ernst added a comment -

          Sorry about that Uwe. This new patch adds back a test to check the build version against Version.LATEST.

          Show
          Ryan Ernst added a comment - Sorry about that Uwe. This new patch adds back a test to check the build version against Version.LATEST .
          Hide
          Uwe Schindler added a comment -

          Thanks!

          +1 to commit!

          One small thing: can we make Version class final?

          Show
          Uwe Schindler added a comment - Thanks! +1 to commit! One small thing: can we make Version class final?
          Hide
          Ryan Ernst added a comment -

          Ok, one more patch with Version made final, version.luceneMatchVersion set to version.base, and the ant version now including the bugfix version as the comment described.

          Show
          Ryan Ernst added a comment - Ok, one more patch with Version made final, version.luceneMatchVersion set to version.base , and the ant version now including the bugfix version as the comment described.
          Hide
          ASF subversion and git services added a comment -

          Commit 1618263 from Ryan Ernst in branch 'dev/trunk'
          [ https://svn.apache.org/r1618263 ]

          LUCENE-5850: Made Version handling more robust and extensible

          Show
          ASF subversion and git services added a comment - Commit 1618263 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1618263 ] LUCENE-5850 : Made Version handling more robust and extensible
          Hide
          ASF subversion and git services added a comment -

          Commit 1618282 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1618282 ]

          LUCENE-5850: Remove useless checks and properties

          Show
          ASF subversion and git services added a comment - Commit 1618282 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1618282 ] LUCENE-5850 : Remove useless checks and properties
          Hide
          Uwe Schindler added a comment -

          Hi Ryan,
          I added another commit removing some useless (now duplicate) checks in common-build.xml (we discussed yesterday on Hangouts...)

          Show
          Uwe Schindler added a comment - Hi Ryan, I added another commit removing some useless (now duplicate) checks in common-build.xml (we discussed yesterday on Hangouts...)
          Hide
          ASF subversion and git services added a comment -

          Commit 1618309 from Ryan Ernst in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1618309 ]

          LUCENE-5850: Made Version handling more robust and extensible

          Show
          ASF subversion and git services added a comment - Commit 1618309 from Ryan Ernst in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1618309 ] LUCENE-5850 : Made Version handling more robust and extensible
          Hide
          Simon Willnauer added a comment -

          Ryan Ernst I looked at the commit and I don't see a hashcode implementation in Version.java but it has an equals one. I think it's bad that none of the tests catches that though I think we should add on. I can get to it tomorrow or so but please feel free to beat me.

          Show
          Simon Willnauer added a comment - Ryan Ernst I looked at the commit and I don't see a hashcode implementation in Version.java but it has an equals one. I think it's bad that none of the tests catches that though I think we should add on. I can get to it tomorrow or so but please feel free to beat me.
          Hide
          Simon Willnauer added a comment - - edited

          here is a simple patch and a test - I am actually disappointed that uwe didn't catch this

          Show
          Simon Willnauer added a comment - - edited here is a simple patch and a test - I am actually disappointed that uwe didn't catch this
          Hide
          Uwe Schindler added a comment -

          I am actually disappointed that uwe didn't catch this

          Krrrrrrr!

          Show
          Uwe Schindler added a comment - I am actually disappointed that uwe didn't catch this Krrrrrrr!
          Hide
          Ryan Ernst added a comment -

          Looks good, thanks for the fix. Just one minor thing:

          assertEquals(Version.parseLeniently(v), v1.hashCode());

          I think this needs a call to hashCode() for the parsed value?

          Show
          Ryan Ernst added a comment - Looks good, thanks for the fix. Just one minor thing: assertEquals(Version.parseLeniently(v), v1.hashCode()); I think this needs a call to hashCode() for the parsed value?
          Hide
          Simon Willnauer added a comment -

          ahh good catch I will fix

          Show
          Simon Willnauer added a comment - ahh good catch I will fix
          Hide
          Simon Willnauer added a comment -

          fixed test bug

          Show
          Simon Willnauer added a comment - fixed test bug
          Hide
          Simon Willnauer added a comment -

          removed @Repeat - I guess it's ready

          Show
          Simon Willnauer added a comment - removed @Repeat - I guess it's ready
          Hide
          ASF subversion and git services added a comment -

          Commit 1618621 from Simon Willnauer in branch 'dev/trunk'
          [ https://svn.apache.org/r1618621 ]

          LUCENE-5850: Add missing hashCode implementation to Version.java

          Show
          ASF subversion and git services added a comment - Commit 1618621 from Simon Willnauer in branch 'dev/trunk' [ https://svn.apache.org/r1618621 ] LUCENE-5850 : Add missing hashCode implementation to Version.java
          Hide
          ASF subversion and git services added a comment -

          Commit 1618639 from Simon Willnauer in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1618639 ]

          LUCENE-5850: Add missing hashCode implementation to Version.java

          Show
          ASF subversion and git services added a comment - Commit 1618639 from Simon Willnauer in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1618639 ] LUCENE-5850 : Add missing hashCode implementation to Version.java

            People

            • Assignee:
              Ryan Ernst
              Reporter:
              Simon Willnauer
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development