Solr
  1. Solr
  2. SOLR-7756

ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.0, 5.0.1, 5.1, 5.2, 5.2.1, 5.2.2
    • Fix Version/s: 5.3
    • Component/s: None
    • Labels:
      None

      Description

      If a term doesn't exist on a shard ExactStatsCache#getPerShardTermStats throws an NullPointerException.

      Attaching a test and a patch shortly.

      1. SOLR-7756.patch
        26 kB
        Varun Thacker
      2. SOLR-7756.patch
        29 kB
        Varun Thacker
      3. SOLR-7756.patch
        28 kB
        Varun Thacker
      4. SOLR-7756.patch
        25 kB
        Varun Thacker
      5. SOLR-7756.patch
        16 kB
        Varun Thacker
      6. SOLR-7756.patch
        4 kB
        Varun Thacker

        Issue Links

          Activity

          Hide
          Varun Thacker added a comment - - edited

          Updated patch with more tests.

          Anshum Gupta can you please review this.

          Show
          Varun Thacker added a comment - - edited Updated patch with more tests. Anshum Gupta can you please review this.
          Hide
          Anshum Gupta added a comment -

          Thanks for the patch Varun. What makes you change the following code to be executed for every term, accidental?

          String termStatsString = StatsUtil.termStatsMapToString(statsMap);
          rb.rsp.add(TERM_STATS_KEY, termStatsString);
          
          Show
          Anshum Gupta added a comment - Thanks for the patch Varun. What makes you change the following code to be executed for every term, accidental? String termStatsString = StatsUtil.termStatsMapToString(statsMap); rb.rsp.add(TERM_STATS_KEY, termStatsString);
          Hide
          Varun Thacker added a comment -

          Updated patch:

          What makes you change the following code to be executed for every term, accidental?

          Reverted this change.

          In ExactStatsCache#mergeToGlobalStats we insert the terms n times ( n number of shards ). Now when a shard doesn't have a term List<Object> terms = nl.getAll(TERMS_KEY) will return empty and not null since NamedList#getAll works like that. Thus depending on the order in which the shards get hit, if the last shard had no terms the terms key was overwritten with blank causing distributed idf to not get calculated correctly. Fixed this in the patch by changing the condition appropriately.

          Minor refactoring

          All tests pass now.

          Show
          Varun Thacker added a comment - Updated patch: What makes you change the following code to be executed for every term, accidental? Reverted this change. In ExactStatsCache#mergeToGlobalStats we insert the terms n times ( n number of shards ). Now when a shard doesn't have a term List<Object> terms = nl.getAll(TERMS_KEY) will return empty and not null since NamedList#getAll works like that. Thus depending on the order in which the shards get hit, if the last shard had no terms the terms key was overwritten with blank causing distributed idf to not get calculated correctly. Fixed this in the patch by changing the condition appropriately. Minor refactoring All tests pass now.
          Hide
          Varun Thacker added a comment -

          New patch where the test picks either LRUStatsCache or ExactStatsCache randomly.

          The patch also fixes the NPE found in LRUStatsCache

          Show
          Varun Thacker added a comment - New patch where the test picks either LRUStatsCache or ExactStatsCache randomly. The patch also fixes the NPE found in LRUStatsCache
          Hide
          Anshum Gupta added a comment -

          Thanks Varun. Looks good. Here are a few suggestions:

          • You should reset solr.test.sys.* system properties during teardown.
          • It'd be good to refactor the test a little bit (nothing pressing)
          • Do we really need 3 shards in the test? I think we can do with just 2 and save time for the test run.

          There's also a bunch of formatting changes that's a part of the patch. I just glanced through it, but in case it's something that's not required, it'd be good to not refactor those.
          P.S: If the current formatting is screwed up, by all means clean it up.

          Show
          Anshum Gupta added a comment - Thanks Varun. Looks good. Here are a few suggestions: You should reset solr.test.sys.* system properties during teardown. It'd be good to refactor the test a little bit (nothing pressing) Do we really need 3 shards in the test? I think we can do with just 2 and save time for the test run. There's also a bunch of formatting changes that's a part of the patch. I just glanced through it, but in case it's something that's not required, it'd be good to not refactor those. P.S: If the current formatting is screwed up, by all means clean it up.
          Hide
          Varun Thacker added a comment -

          You should reset solr.test.sys.* system properties during teardown.

          Fixed

          Do we really need 3 shards in the test? I think we can do with just 2 and save time for the test run.

          Only testSimpleQuery uses 3 shards in which one is empty. The idea of having an extra shard is that shard will not contain terms and expose bugs like SOLR-7818. We could do with 2 shards with if we indexed something like "football world" instead of "football" in one of the documents. But then we'll have no way to compare scores of the two returned docuemnts and we can't do an equality check on the score. Maybe we could once SOLR-7759 is fixed thereby checking debug output and not comparing scores.

          There's also a bunch of formatting changes that's a part of the patch. I just glanced through it, but in case it's something that's not required, it'd be good to not refactor those.

          Some of this got formatted with the code style that was added for Intellij which got fixed in LUCENE-6514.

          In ExactStatsCache#mergeToGlobalStats we insert the terms n times ( n number of shards ). Now when a shard doesn't have a term List<Object> terms = nl.getAll(TERMS_KEY) will return empty and not null since NamedList#getAll works like that. Thus depending on the order in which the shards get hit, if the last shard had no terms the terms key was overwritten with blank causing distributed idf to not get calculated correctly.

          Let's fix this as part of SOLR-7818.

          Show
          Varun Thacker added a comment - You should reset solr.test.sys.* system properties during teardown. Fixed Do we really need 3 shards in the test? I think we can do with just 2 and save time for the test run. Only testSimpleQuery uses 3 shards in which one is empty. The idea of having an extra shard is that shard will not contain terms and expose bugs like SOLR-7818 . We could do with 2 shards with if we indexed something like "football world" instead of "football" in one of the documents. But then we'll have no way to compare scores of the two returned docuemnts and we can't do an equality check on the score. Maybe we could once SOLR-7759 is fixed thereby checking debug output and not comparing scores. There's also a bunch of formatting changes that's a part of the patch. I just glanced through it, but in case it's something that's not required, it'd be good to not refactor those. Some of this got formatted with the code style that was added for Intellij which got fixed in LUCENE-6514 . In ExactStatsCache#mergeToGlobalStats we insert the terms n times ( n number of shards ). Now when a shard doesn't have a term List<Object> terms = nl.getAll(TERMS_KEY) will return empty and not null since NamedList#getAll works like that. Thus depending on the order in which the shards get hit, if the last shard had no terms the terms key was overwritten with blank causing distributed idf to not get calculated correctly. Let's fix this as part of SOLR-7818 .
          Hide
          Anshum Gupta added a comment -

          Thanks for addressing my comments. LGTM now.

          Show
          Anshum Gupta added a comment - Thanks for addressing my comments. LGTM now.
          Hide
          Varun Thacker added a comment -

          One change in this patch. TestDistribIDF#testMultiCollectionQuery is not added here. I'll add that as part of SOLR-7818 since we're tacking them separately.

          I'm running the tests and will commit it after that.

          Show
          Varun Thacker added a comment - One change in this patch. TestDistribIDF#testMultiCollectionQuery is not added here. I'll add that as part of SOLR-7818 since we're tacking them separately. I'm running the tests and will commit it after that.
          Hide
          ASF subversion and git services added a comment -

          Commit 1694182 from Varun Thacker in branch 'dev/trunk'
          [ https://svn.apache.org/r1694182 ]

          SOLR-7756: ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard

          Show
          ASF subversion and git services added a comment - Commit 1694182 from Varun Thacker in branch 'dev/trunk' [ https://svn.apache.org/r1694182 ] SOLR-7756 : ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard
          Hide
          ASF subversion and git services added a comment -

          Commit 1694193 from Varun Thacker in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1694193 ]

          SOLR-7756: ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard (merged trunk r1694182)

          Show
          ASF subversion and git services added a comment - Commit 1694193 from Varun Thacker in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694193 ] SOLR-7756 : ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard (merged trunk r1694182)
          Hide
          Varun Thacker added a comment -

          Thanks Anshum for the review.

          Show
          Varun Thacker added a comment - Thanks Anshum for the review.
          Hide
          ASF subversion and git services added a comment -

          Commit 1694213 from Varun Thacker in branch 'dev/trunk'
          [ https://svn.apache.org/r1694213 ]

          SOLR-7818 SOLR-7756 Added better descriptions in the CHANGES entry for these two issues

          Show
          ASF subversion and git services added a comment - Commit 1694213 from Varun Thacker in branch 'dev/trunk' [ https://svn.apache.org/r1694213 ] SOLR-7818 SOLR-7756 Added better descriptions in the CHANGES entry for these two issues
          Hide
          ASF subversion and git services added a comment -

          Commit 1694214 from Varun Thacker in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1694214 ]

          SOLR-7818 SOLR-7756 Added better descriptions in the CHANGES entry for these two issues (merged from trunk r1694213)

          Show
          ASF subversion and git services added a comment - Commit 1694214 from Varun Thacker in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694214 ] SOLR-7818 SOLR-7756 Added better descriptions in the CHANGES entry for these two issues (merged from trunk r1694213)
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release
          Hide
          Steve Rowe added a comment -

          SOLR-9328 has a reproducible test failure for TestDistribIDF.testMultiCollectionQuery(), which fails on the 5.3.0 release's source, which is when the test was introduced by this issue.

          Show
          Steve Rowe added a comment - SOLR-9328 has a reproducible test failure for TestDistribIDF.testMultiCollectionQuery() , which fails on the 5.3.0 release's source, which is when the test was introduced by this issue.

            People

            • Assignee:
              Varun Thacker
              Reporter:
              Varun Thacker
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development