Lucene - Core
  1. Lucene - Core
  2. LUCENE-3493

Solr reopen on a custom reader doesn't work

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.4
    • Fix Version/s: None
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      When a custom index reader is used with Solr and reopen, the custom reader vanishes after the reopen. It's a bug.

      1. LUCENE-3493.patch
        11 kB
        Jason Rutherglen

        Activity

        Hide
        Jason Rutherglen added a comment -

        Patch with unit test demonstrating the bug.

        The fix required in Lucene is randomly in the patch as well.

        I'll post another patch showing the Lucene fix, allows fixing the bug on the Solr side.

        Show
        Jason Rutherglen added a comment - Patch with unit test demonstrating the bug. The fix required in Lucene is randomly in the patch as well. I'll post another patch showing the Lucene fix, allows fixing the bug on the Solr side.
        Hide
        Mark Miller added a comment -

        I have a couple questions:

        If the bug is in Lucene, shouldn't we write a test at the Lucene level?

        What exactly is the bug? That when you subclass DirectoryReader, it doesn't return that subclass from reopen? If this is the desired behavior, isn't it up to the subclass to override reopen?

        Also, you say the required lucene fix is randomly in the patch, but also that you will post another patch showing the lucene fix - I don't see it in the patch, so I assume its coming, but the only change I see is making some Lucene constructors public - we shouldn't likely do that just for this Solr test I think.

        Show
        Mark Miller added a comment - I have a couple questions: If the bug is in Lucene, shouldn't we write a test at the Lucene level? What exactly is the bug? That when you subclass DirectoryReader, it doesn't return that subclass from reopen? If this is the desired behavior, isn't it up to the subclass to override reopen? Also, you say the required lucene fix is randomly in the patch, but also that you will post another patch showing the lucene fix - I don't see it in the patch, so I assume its coming, but the only change I see is making some Lucene constructors public - we shouldn't likely do that just for this Solr test I think.
        Hide
        Jason Rutherglen added a comment -

        The patch shows the bug only. Which needs a test in Solr. The next patch will show the fix etc. A Lucene test makes sense as well.

        Show
        Jason Rutherglen added a comment - The patch shows the bug only. Which needs a test in Solr. The next patch will show the fix etc. A Lucene test makes sense as well.
        Hide
        Uwe Schindler added a comment -

        This is not a bug at all: Your custom IndexReader has to override reopen() and return your own implementation.

        Show
        Uwe Schindler added a comment - This is not a bug at all: Your custom IndexReader has to override reopen() and return your own implementation.
        Hide
        Jason Rutherglen added a comment -

        Uwe, I'd like to agree with you, however I cannot (because then I wouldn't have had to create an issue!). Look at DR.doOpen* methods. They're private. There's no reason for them to be. They need to be protected, that's in the next patch. Fairly simple. The follow on to this is overriding IW to return custom readers. I had an issue and patch for that a while back. It's best to implement both here, as Lucene 4.x Solr's NRT will show the same problem!

        I think you're right, looks like this could be done be overriding doOpenIfChanged* however, it doesn't make sense to duplicate code!

        Show
        Jason Rutherglen added a comment - Uwe, I'd like to agree with you, however I cannot (because then I wouldn't have had to create an issue!). Look at DR.doOpen* methods. They're private. There's no reason for them to be. They need to be protected, that's in the next patch. Fairly simple. The follow on to this is overriding IW to return custom readers. I had an issue and patch for that a while back. It's best to implement both here, as Lucene 4.x Solr's NRT will show the same problem! I think you're right, looks like this could be done be overriding doOpenIfChanged* however, it doesn't make sense to duplicate code!
        Hide
        Yonik Seeley added a comment -

        Implementing your own IndexReader has always been a very tricky endeavor, esp wrt maintainability... super-expert only. One of the reasons I was glad to get rid of SolrIndexReader (the fragile base class problem).

        Show
        Yonik Seeley added a comment - Implementing your own IndexReader has always been a very tricky endeavor, esp wrt maintainability... super-expert only. One of the reasons I was glad to get rid of SolrIndexReader (the fragile base class problem).
        Hide
        Mark Miller added a comment -

        That clears things up a bit Jason. The title and patch don't really explain the issue.

        as Lucene 4.x Solr's NRT will show the same problem!

        How is that? Solr's NRT does not rely on a custom IndexReader (and if it did, I imagine we would make that properly override doOpenIfChanged, else it would be a bug)?

        Show
        Mark Miller added a comment - That clears things up a bit Jason. The title and patch don't really explain the issue. as Lucene 4.x Solr's NRT will show the same problem! How is that? Solr's NRT does not rely on a custom IndexReader (and if it did, I imagine we would make that properly override doOpenIfChanged, else it would be a bug)?
        Hide
        Jason Rutherglen added a comment -

        Solr's NRT does not rely on a custom IndexReader

        Yikes, logically the custom reader functionality should!

        properly override doOpenIfChanged, else it would be a bug

        It's a bug because there's no way to implement that today. The DirectoryReader is created deep inside of IW.getReader (there's no way to re-implement it's functionality either because of private variable access).

        I think we need a protected method for creating reader in IW. I think though this becomes almost endless because I don't think there's a way to implement a custom IW in Solr.

        Show
        Jason Rutherglen added a comment - Solr's NRT does not rely on a custom IndexReader Yikes, logically the custom reader functionality should! properly override doOpenIfChanged, else it would be a bug It's a bug because there's no way to implement that today. The DirectoryReader is created deep inside of IW.getReader (there's no way to re-implement it's functionality either because of private variable access). I think we need a protected method for creating reader in IW. I think though this becomes almost endless because I don't think there's a way to implement a custom IW in Solr.
        Hide
        Mark Miller added a comment -

        Yikes, logically the custom reader functionality should!

        Okay, I see - you also want your Reader impl to be pulled from IW when using NRT. But as you allude to below, you would need a custom IndexWriter to do that - that is where we get the IndexReader from for NRT.

        That's a scary road to start down currently (or as you say, endless).

        I don't think there's a way to implement a custom IW in Solr.

        Would be pretty advanced stuff, but what we would likely have to do is allow users to provide alternate SolrCoreState impls (currently the DefaultSolrCoreState impl is simply used). This would let you manage what IndexWriter impl was used.

        Show
        Mark Miller added a comment - Yikes, logically the custom reader functionality should! Okay, I see - you also want your Reader impl to be pulled from IW when using NRT. But as you allude to below, you would need a custom IndexWriter to do that - that is where we get the IndexReader from for NRT. That's a scary road to start down currently (or as you say, endless). I don't think there's a way to implement a custom IW in Solr. Would be pretty advanced stuff, but what we would likely have to do is allow users to provide alternate SolrCoreState impls (currently the DefaultSolrCoreState impl is simply used). This would let you manage what IndexWriter impl was used.
        Hide
        Jason Rutherglen added a comment -

        Uwe, I tried your idea. It doesn't work! Here's why: DR.writeLock and DR.segmentInfos are private. Meaning the re-duplicated code because the useful methods aren't protected, cannot access these private variables. Of course one can use reflection but that's just 'atrocious'.

        Show
        Jason Rutherglen added a comment - Uwe, I tried your idea. It doesn't work! Here's why: DR.writeLock and DR.segmentInfos are private. Meaning the re-duplicated code because the useful methods aren't protected, cannot access these private variables. Of course one can use reflection but that's just 'atrocious'.
        Hide
        Jason Rutherglen added a comment -

        One way to solve all of this without subclassing, is to move the IndexReaderFactory to Lucene, integrate it into IW and DR.

        That will be much cleaner than forcing users to subclass, which is a monstrous pain, and will generate excessive unnecessary code in the end.

        Show
        Jason Rutherglen added a comment - One way to solve all of this without subclassing, is to move the IndexReaderFactory to Lucene, integrate it into IW and DR. That will be much cleaner than forcing users to subclass, which is a monstrous pain, and will generate excessive unnecessary code in the end.
        Hide
        Jason Rutherglen added a comment -

        Uwe, check this out on the 3.3 version, what do you think?

        @Override
        public final IndexReader reopen() throws CorruptIndexException, IOException {
          // Preserve current readOnly
          return doReopen(readOnly, null);
        }
        
        @Override
        public final IndexReader reopen(boolean openReadOnly) throws CorruptIndexException, IOException {
          return doReopen(openReadOnly, null);
        }
        
        @Override
        public final IndexReader reopen(final IndexCommit commit) throws CorruptIndexException, IOException {
          return doReopen(true, commit);
        }
        
        Show
        Jason Rutherglen added a comment - Uwe, check this out on the 3.3 version, what do you think? @Override public final IndexReader reopen() throws CorruptIndexException, IOException { // Preserve current readOnly return doReopen(readOnly, null ); } @Override public final IndexReader reopen( boolean openReadOnly) throws CorruptIndexException, IOException { return doReopen(openReadOnly, null ); } @Override public final IndexReader reopen( final IndexCommit commit) throws CorruptIndexException, IOException { return doReopen( true , commit); }
        Hide
        Jason Rutherglen added a comment -

        LUCENE-3498 supercedes this issue as a 100% workable solution.

        Show
        Jason Rutherglen added a comment - LUCENE-3498 supercedes this issue as a 100% workable solution.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jason Rutherglen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development