Solr
  1. Solr
  2. SOLR-3911

Make Directory and DirectoryFactory first class so that the majority of Solr's features work with any custom implementations.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1, Trunk
    • Component/s: None
    • Labels:
      None

      Description

      The biggest issue is that many parts of Solr rely on a local file system based Directory implementation - most notably, replication. This should all be changed to use the Directory and DirectoryFactory abstractions.

      Other parts of the code that count on the local file system for making paths and getting file sizes should also be changed to use Directory and/or DirectoryFactory.

      Original title: Replication should work with any Directory impl, not just local filesystem based Directories.

      I've wanted to do this for a long time - there is no reason replication should not support any directory impl. This will let us use the mockdir for replication tests rather than having to force an FSDir and lose all the extra test checks and simulations. This will improve our testing around replication a lot, and allow custom Directory impls to be used on multi node Solr.

      Expanded scope - full first class support for DirectoryFactory and Directory.

      1. SOLR-3911.patch
        144 kB
        Mark Miller
      2. SOLR-3911.patch
        65 kB
        Mark Miller
      3. SOLR-3911.patch
        39 kB
        Mark Miller

        Issue Links

          Activity

          Mark Miller created issue -
          Hide
          Mark Miller added a comment -

          First draft:

          All tests pass with a few caveats:

          • Replication tests that replicate config files are currently disabled and it's not yet supported.
          • Replication backup command test is disabled and not yet supported.
          • There are a few nocommits around little issues to work out.
          • Still need to add back local FileSystem optimizations - mostly doing rename for a move rather than copy from one Directory impl to another.

          In pretty good shape though.

          Show
          Mark Miller added a comment - First draft: All tests pass with a few caveats: Replication tests that replicate config files are currently disabled and it's not yet supported. Replication backup command test is disabled and not yet supported. There are a few nocommits around little issues to work out. Still need to add back local FileSystem optimizations - mostly doing rename for a move rather than copy from one Directory impl to another. In pretty good shape though.
          Mark Miller made changes -
          Field Original Value New Value
          Attachment SOLR-3911.patch [ 12547449 ]
          Hide
          Mark Miller added a comment -

          Two other things not yet handled:

          • I took out any use of lastModified.
          • I have not yet impl'd getting the directory size from a Directory rather than the filesystem - should be simple enough, right now I just return 0.
          Show
          Mark Miller added a comment - Two other things not yet handled: I took out any use of lastModified. I have not yet impl'd getting the directory size from a Directory rather than the filesystem - should be simple enough, right now I just return 0.
          Hoss Man made changes -
          Link This issue relates to SOLR-3665 [ SOLR-3665 ]
          Hide
          Hoss Man added a comment -

          I have not yet impl'd getting the directory size from a Directory rather than the filesystem - should be simple enough, right now I just return 0.

          mark: can you look into fixing SOLR-3665 when you do this?

          Theres a TODO in CoreAdminHandlerTest related to this because of how CoreAdminHandler currently tries to determine directory size.

          Show
          Hoss Man added a comment - I have not yet impl'd getting the directory size from a Directory rather than the filesystem - should be simple enough, right now I just return 0. mark: can you look into fixing SOLR-3665 when you do this? Theres a TODO in CoreAdminHandlerTest related to this because of how CoreAdminHandler currently tries to determine directory size.
          Hide
          Mark Miller added a comment -

          Yeah, sure.

          I've got the conf tests passing tonight. Just have to do backup and these other clean up issues.

          Show
          Mark Miller added a comment - Yeah, sure. I've got the conf tests passing tonight. Just have to do backup and these other clean up issues.
          Hide
          Mark Miller added a comment -

          New patch.

          All tests are now uncommented and pass.

          Still some more cleanup and checking to do. Especially some hard thinking around where we use File.getCanonical and File.getAbsolute path. There are possibly issues our test cases are not catching.

          Still have a mkdir issue to address properly - right now I just special case for a couple fs based Directory impls.

          sizeOfIndexDirectory now works based on Directory.

          Still have not optimized rename/move for fs based Directory impls.

          Looking good though - this is fairly close.

          Show
          Mark Miller added a comment - New patch. All tests are now uncommented and pass. Still some more cleanup and checking to do. Especially some hard thinking around where we use File.getCanonical and File.getAbsolute path. There are possibly issues our test cases are not catching. Still have a mkdir issue to address properly - right now I just special case for a couple fs based Directory impls. sizeOfIndexDirectory now works based on Directory. Still have not optimized rename/move for fs based Directory impls. Looking good though - this is fairly close.
          Mark Miller made changes -
          Attachment SOLR-3911.patch [ 12548488 ]
          Mark Miller made changes -
          Summary Replication should work with any Directory impl, not just local filesystem based Directories. Make Directory and DirectoryFactory first class so that the majority of Solr's features work with any custom implementations.
          Description I've wanted to do this for a long time - there is no reason replication should not support any directory impl. This will let us use the mockdir for replication tests rather than having to force an FSDir and lose all the extra test checks and simulations. This will improve our testing around replication a lot, and allow custom Directory impls to be used on multi node Solr. The biggest issue is that many parts of Solr rely on a local file system based Directory implementation - most notably, replication. This should all be changed to use the Directory and DirectoryFactory abstractions.

          Other parts of the code that count on the local file system for making paths and getting file sizes should also be changed to use Directory and/or DirectoryFactory.

          Original title: Replication should work with any Directory impl, not just local filesystem based Directories.

          I've wanted to do this for a long time - there is no reason replication should not support any directory impl. This will let us use the mockdir for replication tests rather than having to force an FSDir and lose all the extra test checks and simulations. This will improve our testing around replication a lot, and allow custom Directory impls to be used on multi node Solr.

          Expanded scope - full first class support for DirectoryFactory and Directory.
          Hide
          Mark Miller added a comment -

          I was a little surprised, but even almost all of the solrcloud tests pass without forcing a file system based directory. I thought they probably should, but didn't think I'd end up so lucky.

          Only one cloud test is failing when I don't force a local fs based dir for it - I'll try and look into why soon.

          Show
          Mark Miller added a comment - I was a little surprised, but even almost all of the solrcloud tests pass without forcing a file system based directory. I thought they probably should, but didn't think I'd end up so lucky. Only one cloud test is failing when I don't force a local fs based dir for it - I'll try and look into why soon.
          Hide
          Mark Miller added a comment -

          Only one cloud test is failing when I don't force a local fs based dir for it

          I see what this is about - RamDirectory does not keep it's contents over a restart and when a machine comes up and all of a sudden has no existing index but it still has it's old transaction log files, peer sync recovery than can succeed based on the transaction log, but the node will actually be out of sync.

          Show
          Mark Miller added a comment - Only one cloud test is failing when I don't force a local fs based dir for it I see what this is about - RamDirectory does not keep it's contents over a restart and when a machine comes up and all of a sudden has no existing index but it still has it's old transaction log files, peer sync recovery than can succeed based on the transaction log, but the node will actually be out of sync.
          Mark Miller made changes -
          Link This issue is related to SOLR-3922 [ SOLR-3922 ]
          Hide
          Mark Miller added a comment -

          This issue has been very satisfying. All tests passing. I still force an fs directory for 2 solrcloud tests due to the recovery issue mentioned above. We can probably fix that in another issue.

          Show
          Mark Miller added a comment - This issue has been very satisfying. All tests passing. I still force an fs directory for 2 solrcloud tests due to the recovery issue mentioned above. We can probably fix that in another issue.
          Mark Miller made changes -
          Attachment SOLR-3911.patch [ 12548596 ]
          Hide
          Mark Miller added a comment -

          I think I'm close to committing something. Patch is getting big enough that it will be really annoying to keep up to date - so I'd like to commit and start baking in trunk as soon as possible. I'll post my latest work tomorrow.

          Show
          Mark Miller added a comment - I think I'm close to committing something. Patch is getting big enough that it will be really annoying to keep up to date - so I'd like to commit and start baking in trunk as soon as possible. I'll post my latest work tomorrow.
          Hide
          Mark Miller added a comment -

          I missed converting one of the prop files we write to using a Directory (logReplicationTimeAndConfFiles). This is the file with replication history and stats - obviously no testing for it

          Show
          Mark Miller added a comment - I missed converting one of the prop files we write to using a Directory (logReplicationTimeAndConfFiles). This is the file with replication history and stats - obviously no testing for it
          Hide
          Mark Miller added a comment -

          I'll commit code to write the replication state through the directory in a moment.

          Show
          Mark Miller added a comment - I'll commit code to write the replication state through the directory in a moment.
          Hide
          Mark Miller added a comment -

          'replication stats' I meant - committed.

          Show
          Mark Miller added a comment - 'replication stats' I meant - committed.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1418756

          SOLR-3911: write out replication stats through Directory

          Show
          Commit Tag Bot added a comment - [trunk commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1418756 SOLR-3911 : write out replication stats through Directory
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1418789

          SOLR-3911: sync properties files after write so that they are written out before the directory is closed.

          Show
          Commit Tag Bot added a comment - [trunk commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1418789 SOLR-3911 : sync properties files after write so that they are written out before the directory is closed.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1420231

          SOLR-3911: fix properties file name misspelling and use constant for file name everywhere

          Show
          Commit Tag Bot added a comment - [trunk commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1420231 SOLR-3911 : fix properties file name misspelling and use constant for file name everywhere
          Hide
          Mark Miller added a comment -

          Okay, this is pretty firmed up now - I'm ready to bring it back. Currently, 4x is having more issues with replication than 5x. There are multiple problems showing up in 4x, none of them currently showing up in 5x - so I think it's better we move down to one set of code to track and debug.

          Show
          Mark Miller added a comment - Okay, this is pretty firmed up now - I'm ready to bring it back. Currently, 4x is having more issues with replication than 5x. There are multiple problems showing up in 4x, none of them currently showing up in 5x - so I think it's better we move down to one set of code to track and debug.
          Hide
          Yonik Seeley added a comment -

          +1... merging any of these bug issues targeted for 4.1 (but not yet merged) alone seems really hard at this point. let's just do them together to make sure we don't miss anything. AFAIK, there have been no changes to solr specifically targeted toward 5.

          Show
          Yonik Seeley added a comment - +1... merging any of these bug issues targeted for 4.1 (but not yet merged) alone seems really hard at this point. let's just do them together to make sure we don't miss anything. AFAIK, there have been no changes to solr specifically targeted toward 5.
          Hide
          Mark Miller added a comment -

          Okay, makes sense. There should only be the lazy cores issue other this this and custom hashing. Anything else that is different should be a mistake.

          Show
          Mark Miller added a comment - Okay, makes sense. There should only be the lazy cores issue other this this and custom hashing. Anything else that is different should be a mistake.
          Hide
          Commit Tag Bot added a comment -
          Show
          Commit Tag Bot added a comment - [branch_4x commit] Yonik Seeley http://svn.apache.org/viewvc?view=revision&revision=1420992 SOLR-2592 : SOLR-1028 : SOLR-3922 : SOLR-3911 : sync trunk with 4x
          Hide
          Mark Miller added a comment -

          This is done.

          Show
          Mark Miller added a comment - This is done.
          Mark Miller made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Markus Jelsma made changes -
          Link This issue is related to SOLR-4187 [ SOLR-4187 ]
          Hide
          Mark Miller added a comment -

          I think this needs a little more work. Now that we write small meta data files through the directory, heavy directory impls might be used because they are designed for index directories. I think we need to be able to do something like pass a context that indicates if the dir will be used for an index or meta data files and then a dir factory could return different impls depending.

          Show
          Mark Miller added a comment - I think this needs a little more work. Now that we write small meta data files through the directory, heavy directory impls might be used because they are designed for index directories. I think we need to be able to do something like pass a context that indicates if the dir will be used for an index or meta data files and then a dir factory could return different impls depending.
          Mark Miller made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1432670

          SOLR-3911: pass a context flag so that impls can use different strategies for index directories vs meta data file directories

          Show
          Commit Tag Bot added a comment - [trunk commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1432670 SOLR-3911 : pass a context flag so that impls can use different strategies for index directories vs meta data file directories
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1432671

          SOLR-3911: pass a context flag so that impls can use different strategies for index directories vs meta data file directories

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1432671 SOLR-3911 : pass a context flag so that impls can use different strategies for index directories vs meta data file directories
          Mark Miller made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Steve Rowe made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1440508

          SOLR-3911: Replicate after startup option would not replicate until the IndexWriter was lazily opened.

          Show
          Commit Tag Bot added a comment - [trunk commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1440508 SOLR-3911 : Replicate after startup option would not replicate until the IndexWriter was lazily opened.
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Mark Robert Miller
          http://svn.apache.org/viewvc?view=revision&revision=1440510

          SOLR-3911: Replicate after startup option would not replicate until the IndexWriter was lazily opened.

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1440510 SOLR-3911 : Replicate after startup option would not replicate until the IndexWriter was lazily opened.
          Mark Miller made changes -
          Link This issue is related to SOLR-4380 [ SOLR-4380 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          71d 6h 19m 1 Mark Miller 13/Dec/12 03:52
          Resolved Resolved Reopened Reopened
          31d 13h 5m 1 Mark Miller 13/Jan/13 16:57
          Reopened Reopened Resolved Resolved
          11h 1 Mark Miller 14/Jan/13 03:57
          Resolved Resolved Closed Closed
          10d 2h 35m 1 Steve Rowe 24/Jan/13 06:33

            People

            • Assignee:
              Mark Miller
              Reporter:
              Mark Miller
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development