Lucene - Core
  1. Lucene - Core
  2. LUCENE-1558

Make IndexReader/Searcher ctors readOnly=true by default

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 3.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Another "change the defaults" in 3.0.

      Right now you get a read/write reader from IndexReader.open and new IndexSearcher(...), and reserving the right to write causes thread contention (on isDeleted).

      In 3.0 let's make readOnly reader the default, but still allow opening a read/write IndexReader.

      1. LUCENE-1558.patch
        3 kB
        Michael McCandless

        Activity

        Hide
        Uwe Schindler added a comment -

        We have deprecated all these methods and I removed them yesterday. Do you want to put them back again?

        Show
        Uwe Schindler added a comment - We have deprecated all these methods and I removed them yesterday. Do you want to put them back again?
        Hide
        Michael McCandless added a comment -

        Do you want to put them back again?

        I think we should? Ie, "readOnly=true" is the natural default for creating an IndexReader? Seems silly to force people to specify true/false when they create an IndexReader. But maybe wait until we reach closure on the config/builder discussion on java-dev?

        Show
        Michael McCandless added a comment - Do you want to put them back again? I think we should? Ie, "readOnly=true" is the natural default for creating an IndexReader? Seems silly to force people to specify true/false when they create an IndexReader. But maybe wait until we reach closure on the config/builder discussion on java-dev?
        Hide
        Uwe Schindler added a comment -

        Krrrrr, I rewrote all tests to add this parameter

        Show
        Uwe Schindler added a comment - Krrrrr, I rewrote all tests to add this parameter
        Hide
        Michael McCandless added a comment -

        But, the tests can keep that change (being explicit about read-only).

        Show
        Michael McCandless added a comment - But, the tests can keep that change (being explicit about read-only).
        Hide
        Mark Miller added a comment - - edited

        Ah - didn't see this issue and assumed Uwe's change was what we were going with so I updated the javadoc to reflect. If we change again, it will need another tweak (adding back that its the current default).

        edit

        Personally, I'm not so sure its a bad idea making the user specify as it is now - its not much work and will likely head off users wondering why they can't do a write operation after making an IndexReader and not thinking about the param or that its readonly.

        Show
        Mark Miller added a comment - - edited Ah - didn't see this issue and assumed Uwe's change was what we were going with so I updated the javadoc to reflect. If we change again, it will need another tweak (adding back that its the current default). edit Personally, I'm not so sure its a bad idea making the user specify as it is now - its not much work and will likely head off users wondering why they can't do a write operation after making an IndexReader and not thinking about the param or that its readonly.
        Hide
        Michael McCandless added a comment -

        Personally, I'm not so sure its a bad idea making the user specify as it is now

        The thing is, I expect the vast majority of users use readOnly=true, and to those users they would be baffled that IndexReader even has a readOnly=false possibility. "Simple things should be simple". Also, the exception that's thrown if you attempt a write operation against a readOnly reader is pretty darned clear about what's gone wrong:

        throw new UnsupportedOperationException("This IndexReader cannot make any changes to the index (it was opened with readOnly = true)");

        (We can reword it to say something like "You must open the IndexReader with readOnly=false to make changes" or some such).

        In other cases, I would agree we should force sneaky parameters to be explicit on construction, so users think about the choice. EG we did this with maxFieldLength to IndexWriter, because in that case the truncation was silent, actually resulted in losing indexed content, and we saw from our user's that it tripped people up far too often. But I don't think this case fits that same pattern...

        Show
        Michael McCandless added a comment - Personally, I'm not so sure its a bad idea making the user specify as it is now The thing is, I expect the vast majority of users use readOnly=true, and to those users they would be baffled that IndexReader even has a readOnly=false possibility. "Simple things should be simple". Also, the exception that's thrown if you attempt a write operation against a readOnly reader is pretty darned clear about what's gone wrong: throw new UnsupportedOperationException("This IndexReader cannot make any changes to the index (it was opened with readOnly = true)"); (We can reword it to say something like "You must open the IndexReader with readOnly=false to make changes" or some such). In other cases, I would agree we should force sneaky parameters to be explicit on construction, so users think about the choice. EG we did this with maxFieldLength to IndexWriter, because in that case the truncation was silent, actually resulted in losing indexed content, and we saw from our user's that it tripped people up far too often. But I don't think this case fits that same pattern...
        Hide
        Uwe Schindler added a comment -

        I am +/- 0 for adding these defaults to the ctors.

        Mike, do you have a patch? I think IndexSearcher.ctor and IndexReader.open() are the only affected code parts.

        Show
        Uwe Schindler added a comment - I am +/- 0 for adding these defaults to the ctors. Mike, do you have a patch? I think IndexSearcher.ctor and IndexReader.open() are the only affected code parts.
        Hide
        Michael McCandless added a comment -

        Patch attached, also removing a little dead code from removing deprecations.

        Show
        Michael McCandless added a comment - Patch attached, also removing a little dead code from removing deprecations.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development