Lucene - Core
  1. Lucene - Core
  2. LUCENE-5223

IndexUpgrader (4.4.0) fails when -verbose is not set, or when any value of -dir-impl is specified

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.4
    • Fix Version/s: 4.5, 6.0
    • Component/s: core/index
    • Labels:
      None
    • Environment:

      Linux

    • Lucene Fields:
      New

      Description

      Here it fails because -verbose is not set:

      $ java -cp ./lucene-core-4.4-SNAPSHOT.jar org.apache.lucene.index.IndexUpgrader ./INDEX
      Exception in thread "main" java.lang.IllegalArgumentException: printStream must not be null
      at org.apache.lucene.index.IndexWriterConfig.setInfoStream(IndexWriterConfig.java:514)
      at org.apache.lucene.index.IndexUpgrader.<init>(IndexUpgrader.java:126)
      at org.apache.lucene.index.IndexUpgrader.main(IndexUpgrader.java:109)

      Here it works with -verbose set:

      $ java -cp ./lucene-core-4.4-SNAPSHOT.jar org.apache.lucene.index.IndexUpgrader -verbose ./INDEX
      IFD 0 [Mon Sep 16 18:25:53 PDT 2013; main]: init: current segments file is "segments_5"; deletionPolicy=org.apache.lucene.index.KeepOnlyLastCommitDeletionPolicy@42698403

      ...

      IW 0 [Mon Sep 16 18:25:53 PDT 2013; main]: at close: _2(4.4):C4

      1. LUCENE-5223.patch
        7 kB
        Hoss Man
      2. LUCENE-5223.patch
        6 kB
        Hoss Man

        Activity

        Hide
        Hoss Man added a comment -

        As mentioned to uwe on irc, i took a stab at improving hte tests for IndexUpgrader – we weren't doing any testing of the command line parsing logic, and only one of the class constructors was being tested.

        this attached patch includes the new tests, as well as a quick attempt at fixing the issue reported. it also fixes another issue discoverd by the test: "-dir-impl" aparently didn't work at all, because it would be interpreted as the name of the directory and then the subsequent option value would be considered bogus and cause the usage to be printed.

        This patch still has some problems...

        1) since it now randomly tests "verbose" mode, it's really verbose. i don't have any good suggestions here other them to create a static variable that defaults to System.out when the code runs normally, but the tests could set it to some MockOutputStream in a @BeforeClass

        2) there are some nocommit lines relate to how we randomize the "-dir-impl" option ... it's really kludgy now, but it's the best i could come up with w/o making changes to LuceneTestCase ... hopefully someone else has a suggestion.

        3) as i mentioned on irc, i'm not convinced that IndexWriterConfig doesn't also need fixed...

        • IndexWriterConfig.setInfoStream(InfoStream) javadocs say "If non-null, ... will be printed to this." but it throws an error if you try to set it to the value of null you get an error – why doesn't it just implicitly use NO_OUTPUT if the arg is null? why don't the javadocs mention NO_OUTPUT?
        • IndexWriterConfig.setInfoStream(PrintStream) javadocs just say it's a convinience wrapper using PrintStreamInfoStream, with no mention of null at all – even if setInfoStream(InfoStream) is going to be a hard-ass about null, why can't setInfoStream(PrintStream) implicitly use NO_OUTPUT when it's arg is null?
        Show
        Hoss Man added a comment - As mentioned to uwe on irc, i took a stab at improving hte tests for IndexUpgrader – we weren't doing any testing of the command line parsing logic, and only one of the class constructors was being tested. this attached patch includes the new tests, as well as a quick attempt at fixing the issue reported. it also fixes another issue discoverd by the test: "-dir-impl" aparently didn't work at all, because it would be interpreted as the name of the directory and then the subsequent option value would be considered bogus and cause the usage to be printed. This patch still has some problems... 1) since it now randomly tests "verbose" mode, it's really verbose. i don't have any good suggestions here other them to create a static variable that defaults to System.out when the code runs normally, but the tests could set it to some MockOutputStream in a @BeforeClass 2) there are some nocommit lines relate to how we randomize the "-dir-impl" option ... it's really kludgy now, but it's the best i could come up with w/o making changes to LuceneTestCase ... hopefully someone else has a suggestion. 3) as i mentioned on irc, i'm not convinced that IndexWriterConfig doesn't also need fixed... IndexWriterConfig.setInfoStream(InfoStream) javadocs say "If non-null, ... will be printed to this." but it throws an error if you try to set it to the value of null you get an error – why doesn't it just implicitly use NO_OUTPUT if the arg is null? why don't the javadocs mention NO_OUTPUT? IndexWriterConfig.setInfoStream(PrintStream) javadocs just say it's a convinience wrapper using PrintStreamInfoStream, with no mention of null at all – even if setInfoStream(InfoStream) is going to be a hard-ass about null, why can't setInfoStream(PrintStream) implicitly use NO_OUTPUT when it's arg is null?
        Hide
        Uwe Schindler added a comment - - edited

        Hi Hoss,
        thanks for the patch. You can assign the issue to yourself. I just took it, because I wrote the code originally, so I wanted find out what changed.

        In that case the change to Luecen 3 was, that IndexWriterConfig no longer allows null as InfoStream, but instead requires NO_OUTPUT constant (which is a good thing). While doing this change, the code in IndexUpgrader was unfortunately not upgraded. Your new tests are fine. The random directory is OK.

        I don't think that we should allow null in IndexWriterConfig. I don't like this behaviour in Solr (e.g., SolrResourceLoader and a lot of other classes) to use crazy defaults if somewhere null is passed. Always be explicit.

        The problem is a missing @NotNull annotation, like Java 8 provides (does it?).

        We should state in the javadocs that null is not allowed.

        Show
        Uwe Schindler added a comment - - edited Hi Hoss, thanks for the patch. You can assign the issue to yourself. I just took it, because I wrote the code originally, so I wanted find out what changed. In that case the change to Luecen 3 was, that IndexWriterConfig no longer allows null as InfoStream, but instead requires NO_OUTPUT constant (which is a good thing). While doing this change, the code in IndexUpgrader was unfortunately not upgraded. Your new tests are fine. The random directory is OK. I don't think that we should allow null in IndexWriterConfig. I don't like this behaviour in Solr (e.g., SolrResourceLoader and a lot of other classes) to use crazy defaults if somewhere null is passed. Always be explicit. The problem is a missing @NotNull annotation, like Java 8 provides (does it?). We should state in the javadocs that null is not allowed.
        Hide
        Hoss Man added a comment -

        update patch with doc improvements to IWC.setInfoStream and change nocommits to TODO since uwe didn't seem concerned by them

        Show
        Hoss Man added a comment - update patch with doc improvements to IWC.setInfoStream and change nocommits to TODO since uwe didn't seem concerned by them
        Hide
        ASF subversion and git services added a comment -

        Commit 1524521 from hossman@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1524521 ]

        LUCENE-5223: Fixed IndexUpgrader command line parsing: -verbose is not required and -dir-impl option now works correctly

        Show
        ASF subversion and git services added a comment - Commit 1524521 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1524521 ] LUCENE-5223 : Fixed IndexUpgrader command line parsing: -verbose is not required and -dir-impl option now works correctly
        Hide
        ASF subversion and git services added a comment -

        Commit 1524529 from hossman@apache.org in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1524529 ]

        LUCENE-5223: Fixed IndexUpgrader command line parsing: -verbose is not required and -dir-impl option now works correctly (merge r1524521)

        Show
        ASF subversion and git services added a comment - Commit 1524529 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1524529 ] LUCENE-5223 : Fixed IndexUpgrader command line parsing: -verbose is not required and -dir-impl option now works correctly (merge r1524521)
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5223: fix compile

        Show
        ASF subversion and git services added a comment - Commit 1524592 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1524592 ] LUCENE-5223 : fix compile
        Hide
        ASF subversion and git services added a comment -

        Commit 1524745 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1524745 ]

        LUCENE-5223: backport to 4.5

        Show
        ASF subversion and git services added a comment - Commit 1524745 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1524745 ] LUCENE-5223 : backport to 4.5
        Hide
        ASF subversion and git services added a comment -

        Commit 1524748 from Adrien Grand in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1524748 ]

        LUCENE-5223: Backport to 4.5.

        Show
        ASF subversion and git services added a comment - Commit 1524748 from Adrien Grand in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1524748 ] LUCENE-5223 : Backport to 4.5.
        Hide
        ASF subversion and git services added a comment -

        Commit 1524754 from Adrien Grand in branch 'dev/branches/lucene_solr_4_5'
        [ https://svn.apache.org/r1524754 ]

        LUCENE-5223: backport to 4.5.

        Show
        ASF subversion and git services added a comment - Commit 1524754 from Adrien Grand in branch 'dev/branches/lucene_solr_4_5' [ https://svn.apache.org/r1524754 ] LUCENE-5223 : backport to 4.5.
        Hide
        Adrien Grand added a comment -

        4.5 release -> bulk close

        Show
        Adrien Grand added a comment - 4.5 release -> bulk close

          People

          • Assignee:
            Hoss Man
            Reporter:
            Bruce Karsh
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development