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

Do not cache merge or 'read once' contexts in the hdfs block cache.

    Details

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

      Description

      Generally the block cache will not be large enough to contain the whole index and merges can thrash the cache for queries. Even if we still look in the cache, we should not populate it.

      1. SOLR-8856.patch
        4 kB
        Mark Miller
      2. SOLR-8856.patch
        4 kB
        Mark Miller
      3. SOLR-8856.patch
        1 kB
        Mark Miller

        Activity

        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S5 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-8856: Remove extra changes entry.

        Show
        jira-bot ASF subversion and git services added a comment - Commit bfc6dcf92ea49713ecdaa14543ffaa07c62de807 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bfc6dcf ] SOLR-8856 : Remove extra changes entry.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 461c9b4fef5c173c96ac2aa68daa87ef2bed2c16 in lucene-solr's branch refs/heads/branch_6x from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=461c9b4 ]

        SOLR-8856: Do not cache merge or 'read once' contexts in the hdfs block cache.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 461c9b4fef5c173c96ac2aa68daa87ef2bed2c16 in lucene-solr's branch refs/heads/branch_6x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=461c9b4 ] SOLR-8856 : Do not cache merge or 'read once' contexts in the hdfs block cache.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Not resolved, has not been backported yet.

        I'll fix the extra entry.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Not resolved, has not been backported yet. I'll fix the extra entry.
        Hide
        markus17 Markus Jelsma added a comment - - edited

        Hey - is this resolved? This is still open and it mentioned twice in 6.1.0's changes.txt

        73 Optimizations
        74 ----------------------
        75 * SOLR-8722: Don't force a full ZkStateReader refresh on every Overseer operation.
        76 (Scott Blum via shalin)
        77
        78 * SOLR-8856: Do not cache merge or 'read once' contexts in the hdfs block cache. (Mark Miller)
        79
        80 * SOLR-8745: Deprecate costly ZkStateReader.updateClusterState(), replace with a narrow
        81 forceUpdateCollection(collection) (Scott Blum via shalin)
        82
        83 * SOLR-8856: Do not cache merge or 'read once' contexts in the hdfs block cache. (Mark Miller, Mike Drob)

        Also, it has no fixversion set.

        Show
        markus17 Markus Jelsma added a comment - - edited Hey - is this resolved? This is still open and it mentioned twice in 6.1.0's changes.txt 73 Optimizations 74 ---------------------- 75 * SOLR-8722 : Don't force a full ZkStateReader refresh on every Overseer operation. 76 (Scott Blum via shalin) 77 78 * SOLR-8856 : Do not cache merge or 'read once' contexts in the hdfs block cache. (Mark Miller) 79 80 * SOLR-8745 : Deprecate costly ZkStateReader.updateClusterState(), replace with a narrow 81 forceUpdateCollection(collection) (Scott Blum via shalin) 82 83 * SOLR-8856 : Do not cache merge or 'read once' contexts in the hdfs block cache. (Mark Miller, Mike Drob) Also, it has no fixversion set.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-8856: Do not cache merge or 'read once' contexts in the hdfs block cache.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 574da7667f571e0c9e0527b14e9dec14415200f6 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=574da76 ] SOLR-8856 : Do not cache merge or 'read once' contexts in the hdfs block cache.
        Hide
        mdrob Mike Drob added a comment -

        Then yea, I see no reason to keep the old constructor.

        Show
        mdrob Mike Drob added a comment - Then yea, I see no reason to keep the old constructor.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        What is our compatibility promise? Do we need to maintain the old constructor that is no longer being used?

        Solr does not promise back compat on low level Java API's. Just HTTP and our best effort on solrj APIs.

        But also, this is a patch for trunk which has no back compat promises.

        maybe it is time to use a builder or a configuration object?

        Not really worth it IMO - this is not supposed to be a user facing class in Solr and I don't see it being instantiated somewhere else anytime soon either.

        Show
        markrmiller@gmail.com Mark Miller added a comment - What is our compatibility promise? Do we need to maintain the old constructor that is no longer being used? Solr does not promise back compat on low level Java API's. Just HTTP and our best effort on solrj APIs. But also, this is a patch for trunk which has no back compat promises. maybe it is time to use a builder or a configuration object? Not really worth it IMO - this is not supposed to be a user facing class in Solr and I don't see it being instantiated somewhere else anytime soon either.
        Hide
        mdrob Mike Drob added a comment -

        + // we don't cache on merges or when only reading once

        This comment needs to be updated to reflect configuration.

        + if (cacheMerges)

        Unknown macro: {+ return true;+ }

        + return false;

        ...

        + if (cacheReadOnce)

        Unknown macro: {+ return true;+ }

        + return false;

        Why not return cacheMerges and return cacheReadOnce?

        + public BlockDirectory(String dirName, Directory directory, Cache cache,
        + Set<String> blockCacheFileTypes, boolean blockCacheReadEnabled,
        + boolean blockCacheWriteEnabled, boolean cacheMerges, boolean cacheReadOnce) throws IOException {

        What is our compatibility promise? Do we need to maintain the old constructor that is no longer being used?

        Alternatively, we're starting to get a lot of parameters here, maybe it is time to use a builder or a configuration object? (Can be a follow-on issue.)

        Show
        mdrob Mike Drob added a comment - + // we don't cache on merges or when only reading once This comment needs to be updated to reflect configuration. + if (cacheMerges) Unknown macro: {+ return true;+ } + return false; ... + if (cacheReadOnce) Unknown macro: {+ return true;+ } + return false; Why not return cacheMerges and return cacheReadOnce ? + public BlockDirectory(String dirName, Directory directory, Cache cache, + Set<String> blockCacheFileTypes, boolean blockCacheReadEnabled, + boolean blockCacheWriteEnabled, boolean cacheMerges, boolean cacheReadOnce) throws IOException { What is our compatibility promise? Do we need to maintain the old constructor that is no longer being used? Alternatively, we're starting to get a lot of parameters here, maybe it is time to use a builder or a configuration object? (Can be a follow-on issue.)
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Adds configuration.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Adds configuration.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Next will look at making this configurable - I think this new patch is the right default though.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Next will look at making this configurable - I think this new patch is the right default though.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Not caching on read once should help too - we will no longer run replication of indexes through the block cache.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Not caching on read once should help too - we will no longer run replication of indexes through the block cache.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        First patch.

        Show
        markrmiller@gmail.com Mark Miller added a comment - First patch.

          People

          • Assignee:
            markrmiller@gmail.com Mark Miller
            Reporter:
            markrmiller@gmail.com Mark Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development