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

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

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.5, 7.0
    • Component/s: search
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      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.

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

        Activity

        Hide
        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.

        Show
        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.
        Hide
        sarkaramrit2@gmail.com Amrit Sarkar added a comment -

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

        Show
        sarkaramrit2@gmail.com Amrit Sarkar added a comment - Added initialization check test-cases in the updated patch. Feedback will be appreciated.
        Hide
        thetaphi 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.

        Show
        thetaphi 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.
        Hide
        sarkaramrit2@gmail.com Amrit Sarkar added a comment -

        Uwe Schindler 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.

        Show
        sarkaramrit2@gmail.com Amrit Sarkar added a comment - Uwe Schindler 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.
        Hide
        thetaphi 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!

        Show
        thetaphi 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!
        Hide
        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.

        Show
        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.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

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

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

        Commit 68eb20c5d0d026e6ce486b4fb09fa812ee738ca9 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler
        [ 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
        Show
        jira-bot ASF subversion and git services added a comment - Commit 68eb20c5d0d026e6ce486b4fb09fa812ee738ca9 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler [ 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
        Hide
        thetaphi Uwe Schindler added a comment -

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

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development