Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10158

MMapDirectoryFactory support for "preload" option (LUCENE-6549)

Details

    • Improvement
    • Status: Closed
    • Trivial
    • Resolution: Fixed
    • None
    • 6.5, 7.0
    • search
    • None

    Description

      Lucene 5.3 added a new preload option to MMapDirectory (see LUCENE-6549)
      MMapDirectoryFactory needs to be updated to offer this as a config option.

      Attachments

        1. SOLR-10158.patch
          8 kB
          Amrit Sarkar
        2. SOLR-10158-no-test-cases.patch
          2 kB
          Amrit Sarkar

        Activity

          sarkaramrit2@gmail.com Amrit Sarkar added a comment -

          SOLR-10158.patch uploaded.

          Added a new boolean init param "preload" in MMapDirectoryFactory. Very trivial.

          Though loading the data at startup-time is not favorable as discussed in LUCENE-6549, but anybody has the option now.

          sarkaramrit2@gmail.com Amrit Sarkar added a comment - SOLR-10158 .patch uploaded. Added a new boolean init param "preload" in MMapDirectoryFactory. Very trivial. Though loading the data at startup-time is not favorable as discussed in LUCENE-6549 , but anybody has the option now.
          sarkaramrit2@gmail.com Amrit Sarkar added a comment -

          Added initialization check test-cases in the updated patch. Feedback will be appreciated.

          sarkaramrit2@gmail.com Amrit Sarkar added a comment - Added initialization check test-cases in the updated patch. Feedback will be appreciated.
          uschindler Uwe Schindler added a comment -

          I'd not add a separate test for it. If you really want to do this, safe it using the special LuceneTestCase.assumeWorkingMmap method, because otherwise tests may fail on some operating systems or windows. But I'd really remove the test, it tests nothing just that parsing of config options works, but this is tested at other places already.

          uschindler Uwe Schindler added a comment - I'd not add a separate test for it. If you really want to do this, safe it using the special LuceneTestCase.assumeWorkingMmap method, because otherwise tests may fail on some operating systems or windows. But I'd really remove the test, it tests nothing just that parsing of config options works, but this is tested at other places already.
          sarkaramrit2@gmail.com Amrit Sarkar added a comment -

          thetaphi I agree, the patch is very trivial and I put the test-cases for the sake of it. We can use the old-patch where we added 4 lines/optional parameter. LuceneTestCase.assumeWorkingMmap method is testing the directory, which is already tested, not the factory.

          Uploaded a patch with no test-cases.

          sarkaramrit2@gmail.com Amrit Sarkar added a comment - thetaphi I agree, the patch is very trivial and I put the test-cases for the sake of it. We can use the old-patch where we added 4 lines/optional parameter. LuceneTestCase.assumeWorkingMmap method is testing the directory, which is already tested, not the factory. Uploaded a patch with no test-cases.
          uschindler Uwe Schindler added a comment -

          LuceneTestCase.assumeWorkingMmap method is testing the directory, which is already tested, not the factory.

          You misunderstood that. The assumeWorkingMmap is a check that unmapping works at all. If not, it disables the test - and that is what I wanted. Your test does not work if unmapping does not work, so it must be disabled if this is the case (On windows without unmapping the test Framework will complain about still open files after core startup...). assumeWorkingMmap takes care of that.

          Nevertheless, the test is not needed, lets go without it! I will commit the no-test patch soon. Thanks!

          uschindler Uwe Schindler added a comment - LuceneTestCase.assumeWorkingMmap method is testing the directory, which is already tested, not the factory. You misunderstood that. The assumeWorkingMmap is a check that unmapping works at all. If not, it disables the test - and that is what I wanted. Your test does not work if unmapping does not work, so it must be disabled if this is the case (On windows without unmapping the test Framework will complain about still open files after core startup...). assumeWorkingMmap takes care of that. Nevertheless, the test is not needed, lets go without it! I will commit the no-test patch soon. Thanks!
          sarkaramrit2@gmail.com Amrit Sarkar added a comment -

          Uwe, thank you for the clarification. I certainly didn't focus enough to understand the significance of the test-method you mentioned.

          Appreciate your feedback too. Thanks.

          sarkaramrit2@gmail.com Amrit Sarkar added a comment - Uwe, thank you for the clarification. I certainly didn't focus enough to understand the significance of the test-method you mentioned. Appreciate your feedback too. Thanks.

          Commit ea37b9ae870257c943bdc8c2896f1238a4dc94b6 in lucene-solr's branch refs/heads/master from thetaphi
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ea37b9a ]

          SOLR-10158: Add support for "preload" option in MMapDirectoryFactory

          jira-bot ASF subversion and git services added a comment - Commit ea37b9ae870257c943bdc8c2896f1238a4dc94b6 in lucene-solr's branch refs/heads/master from thetaphi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ea37b9a ] SOLR-10158 : Add support for "preload" option in MMapDirectoryFactory

          Commit 68eb20c5d0d026e6ce486b4fb09fa812ee738ca9 in lucene-solr's branch refs/heads/branch_6x from thetaphi
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=68eb20c ]

          SOLR-10158: Add support for "preload" option in MMapDirectoryFactory

          1. Conflicts:
          2. solr/CHANGES.txt
          jira-bot ASF subversion and git services added a comment - Commit 68eb20c5d0d026e6ce486b4fb09fa812ee738ca9 in lucene-solr's branch refs/heads/branch_6x from thetaphi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=68eb20c ] SOLR-10158 : Add support for "preload" option in MMapDirectoryFactory Conflicts: solr/CHANGES.txt
          uschindler Uwe Schindler added a comment -

          Thanks, I committed the patch. Will be in Solr 6.5!

          uschindler Uwe Schindler added a comment - Thanks, I committed the patch. Will be in Solr 6.5!

          People

            uschindler Uwe Schindler
            sarkaramrit2@gmail.com Amrit Sarkar
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: