Solr
  1. Solr
  2. SOLR-3316

Distributed Grouping fails in some scenarios.

    Details

      Description

      During a distributed grouping request, if rows is set to 0 a 500 error is returned.

      If groups are unique to a shard and the row count is set to 1, then the matches number is only the matches from one shard.

      I've put together a failing test.

      1. SOLR-3316.patch
        25 kB
        Martijn van Groningen
      2. SOLR-3316-3x.patch
        24 kB
        Martijn van Groningen
      3. SOLR-3316-3x.patch
        24 kB
        Martijn van Groningen
      4. TestDistributedGrouping.java.patch
        5 kB
        Cody Young

        Issue Links

          Activity

          Hide
          Martijn van Groningen added a comment -

          Thanks for creating the test case!

          I dived into the issue and I figured out why the matches is wrong when the top groups aren't retrieved from all shards. The total hit count (matches) is now computed in the first phase instead of in the second phase. The test passes now.

          What is odd is that the grouping request where stats is being used even fails when grouping is disabled...

          I think this issue also occur in all 3.x versions.

          Show
          Martijn van Groningen added a comment - Thanks for creating the test case! I dived into the issue and I figured out why the matches is wrong when the top groups aren't retrieved from all shards. The total hit count (matches) is now computed in the first phase instead of in the second phase. The test passes now. What is odd is that the grouping request where stats is being used even fails when grouping is disabled... I think this issue also occur in all 3.x versions.
          Hide
          Cody Young added a comment -

          That did it! All working now.

          Thanks for you help and I'm glad I could contribute.

          Show
          Cody Young added a comment - That did it! All working now. Thanks for you help and I'm glad I could contribute.
          Hide
          Martijn van Groningen added a comment -

          This issue also occurs in previous 3x releases. Attached a patch that fixes that.

          Show
          Martijn van Groningen added a comment - This issue also occurs in previous 3x releases. Attached a patch that fixes that.
          Hide
          Robert Muir added a comment -

          Some parts of the patch make it difficult to review:

          • why does SimpleEndResultTransformer.transform lose its @Override?
          • why does GroupedEndResultTransformer/SimpleEndResultTransformer.transform gain an {@inheritDoc}

            ? I'm not sure this will really do what you expect, because it extends a package-private method (EndResultTransformer.inheritDoc), which we don't generate javadocs for (only public, protected).

          Show
          Robert Muir added a comment - Some parts of the patch make it difficult to review: why does SimpleEndResultTransformer.transform lose its @Override? why does GroupedEndResultTransformer/SimpleEndResultTransformer.transform gain an {@inheritDoc} ? I'm not sure this will really do what you expect, because it extends a package-private method (EndResultTransformer.inheritDoc), which we don't generate javadocs for (only public, protected).
          Hide
          Martijn van Groningen added a comment -

          Changed priority to blocker to include this bug fix into the 3.6 release. I think the changes are safe. I ran the build several times (with java 5). The javadocs task doesn't throw any errors. I also ran the distributed grouping test with multiplier of 3:
          ant test-core -Dtests.multiplier=3 -Dtestcase=TestDistributedGrouping

          If others think the changes aren't safe then the block priority can be removed.

          Show
          Martijn van Groningen added a comment - Changed priority to blocker to include this bug fix into the 3.6 release. I think the changes are safe. I ran the build several times (with java 5). The javadocs task doesn't throw any errors. I also ran the distributed grouping test with multiplier of 3: ant test-core -Dtests.multiplier=3 -Dtestcase=TestDistributedGrouping If others think the changes aren't safe then the block priority can be removed.
          Hide
          Martijn van Groningen added a comment -

          why does SimpleEndResultTransformer.transform lose its @Override?

          I can change that back. My personal preference is to use @inheritDoc instead of @Override for method that implements from an interface.

          why does GroupedEndResultTransformer/SimpleEndResultTransformer.transform gain an

          Unknown macro: {@inheritDoc}

          ? I'm not sure this will really do what you expect, because it extends a package-private method (EndResultTransformer.inheritDoc), which we don't generate javadocs for (only public, protected).

          EndResultTransformer is an interface. The default is visibility public for methods, so we want to keep the javadoc, right?

          Show
          Martijn van Groningen added a comment - why does SimpleEndResultTransformer.transform lose its @Override? I can change that back. My personal preference is to use @inheritDoc instead of @Override for method that implements from an interface. why does GroupedEndResultTransformer/SimpleEndResultTransformer.transform gain an Unknown macro: {@inheritDoc} ? I'm not sure this will really do what you expect, because it extends a package-private method (EndResultTransformer.inheritDoc), which we don't generate javadocs for (only public, protected). EndResultTransformer is an interface. The default is visibility public for methods, so we want to keep the javadoc, right?
          Hide
          Robert Muir added a comment -

          I can change that back. My personal preference is to use @inheritDoc instead of @Override for method that implements from an interface.

          Thats ok: for 3.6 we cannot have this @Override here anyway.

          EndResultTransformer is an interface. The default is visibility public for methods, so we want to keep the javadoc, right?

          I think i recommend just doing 'ant javadocs' and inspecting build/docs/... and ensuring the javadocs read as you would like.

          If you are satisfied, please commit to trunk only first and others can review while hudson chews on it in trunk: or does this somehow not affect 4.0?!

          Show
          Robert Muir added a comment - I can change that back. My personal preference is to use @inheritDoc instead of @Override for method that implements from an interface. Thats ok: for 3.6 we cannot have this @Override here anyway. EndResultTransformer is an interface. The default is visibility public for methods, so we want to keep the javadoc, right? I think i recommend just doing 'ant javadocs' and inspecting build/docs/... and ensuring the javadocs read as you would like. If you are satisfied, please commit to trunk only first and others can review while hudson chews on it in trunk: or does this somehow not affect 4.0?!
          Hide
          Martijn van Groningen added a comment -

          If you are satisfied, please commit to trunk only first and others can review while hudson chews on it in trunk: or does this somehow not affect 4.0?!

          It does affect trunk. I only did use 4.0 as affect version, since it isn't released. I'll commit to trunk first and see if Hudson likes this change.

          Show
          Martijn van Groningen added a comment - If you are satisfied, please commit to trunk only first and others can review while hudson chews on it in trunk: or does this somehow not affect 4.0?! It does affect trunk. I only did use 4.0 as affect version, since it isn't released. I'll commit to trunk first and see if Hudson likes this change.
          Hide
          Martijn van Groningen added a comment -

          Committed to trunk.

          Show
          Martijn van Groningen added a comment - Committed to trunk.
          Hide
          Martijn van Groningen added a comment -

          Updated patch.

          Show
          Martijn van Groningen added a comment - Updated patch.
          Hide
          Michael McCandless added a comment -

          Patch looks good!

          I guess it's OK to make the hard change to the EndResultTransformer interface... (it's marked @experimental).

          Show
          Michael McCandless added a comment - Patch looks good! I guess it's OK to make the hard change to the EndResultTransformer interface... (it's marked @experimental).
          Hide
          Robert Muir added a comment -

          Given mike's review, i think this should be committed to 3.x

          Show
          Robert Muir added a comment - Given mike's review, i think this should be committed to 3.x
          Hide
          Robert Muir added a comment -

          I'll take the backport.

          Show
          Robert Muir added a comment - I'll take the backport.
          Hide
          Robert Muir added a comment -

          Thanks Cody!

          Show
          Robert Muir added a comment - Thanks Cody!

            People

            • Assignee:
              Martijn van Groningen
              Reporter:
              Cody Young
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development