Solr
  1. Solr
  2. SOLR-8147

FieldFacetAccumulator constructor throws too many exceptions

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 5.0, 6.0
    • Fix Version/s: 5.4, 6.0
    • Labels:
      None
    • Flags:
      Patch

      Description

      The constructor and static create method in FieldFacetAccumulator don't need to throw IOException, just SolrException.

      1. SOLR-8147.patch
        3 kB
        Scott Stults
      2. SOLR-8147.patch
        2 kB
        Scott Stults

        Activity

        Hide
        Scott Stults added a comment -

        Patch removes IOException and just leaves SolrException for the constructor and create method.

        Show
        Scott Stults added a comment - Patch removes IOException and just leaves SolrException for the constructor and create method.
        Hide
        Christine Poerschke added a comment -

        Interesting find. Looking at class usage, currently FacetingAccumulator contains/constructs FieldFacetAccumulator and both classes' constructors are throws IOException - so not sure re: changing only the latter over to throws SolrException then since SolrException is not an IOException (i think).

        Considering the overall class usage and calling chains, might the hasDocValues() check be relocated to somewhere higher up? From a cursory look AnalyticsContentHandler.endElement and AnalyticsRequestFactory.(parse|makeFieldFacet|setFieldFacetParam) construct FieldFacetRequest objects and perhaps the hasDocValues check could happen there i.e. catch the invalid input/bad request when considering request parameters and then within the lower level accumulator object the hasDocValues check goes away and no SolrException need be thrown by the FieldFacetAccumulator.

        Show
        Christine Poerschke added a comment - Interesting find. Looking at class usage, currently FacetingAccumulator contains/constructs FieldFacetAccumulator and both classes' constructors are throws IOException - so not sure re: changing only the latter over to throws SolrException then since SolrException is not an IOException (i think). Considering the overall class usage and calling chains, might the hasDocValues() check be relocated to somewhere higher up? From a cursory look AnalyticsContentHandler.endElement and AnalyticsRequestFactory.(parse|makeFieldFacet|setFieldFacetParam) construct FieldFacetRequest objects and perhaps the hasDocValues check could happen there i.e. catch the invalid input/bad request when considering request parameters and then within the lower level accumulator object the hasDocValues check goes away and no SolrException need be thrown by the FieldFacetAccumulator .
        Hide
        Scott Stults added a comment -

        You're right: SolrException isn't an IOException, and the latter is probably more appropriate given the class hierarchy.

        As far as moving the hasDocValues() check though, I'm conflicted. On the one hand it would be better to catch the problem as early as possible. On the other, locating the check closer to where it's needed reduces code complexity. Which do you think is more valuable in this case?

        Show
        Scott Stults added a comment - You're right: SolrException isn't an IOException, and the latter is probably more appropriate given the class hierarchy. As far as moving the hasDocValues() check though, I'm conflicted. On the one hand it would be better to catch the problem as early as possible. On the other, locating the check closer to where it's needed reduces code complexity. Which do you think is more valuable in this case?
        Hide
        Houston Putman added a comment -

        I agree that the hasDocValues() check should be moved to AnalyticsRequestFactory.(makeFieldFacet|setFieldFacetParam). It shouldn't add any complexity since those methods are where the SchemaField references, which FieldFacetAccumulator uses, originate. Other error checking could be done in a similar manner. For example result() calls in query and range facets could be validated after AnalyticsRequestFactory.parse() is finished, this would greatly reduce the complexity in the (BasicAccumulator|FacetingAccumulator).getResult() and FacetingAccumulator.getQueryResult() methods. There are probably others, but that's the first that popped into my head.

        Show
        Houston Putman added a comment - I agree that the hasDocValues() check should be moved to AnalyticsRequestFactory.(makeFieldFacet|setFieldFacetParam) . It shouldn't add any complexity since those methods are where the SchemaField references, which FieldFacetAccumulator uses, originate. Other error checking could be done in a similar manner. For example result() calls in query and range facets could be validated after AnalyticsRequestFactory.parse() is finished, this would greatly reduce the complexity in the (BasicAccumulator|FacetingAccumulator).getResult() and FacetingAccumulator.getQueryResult() methods. There are probably others, but that's the first that popped into my head.
        Hide
        Christine Poerschke added a comment -

        Okay, so if we're looking at moving not only hasDocValues() but also other error checking in a similar manner out of the accumulator class(es) - how about we keep this JIRA ticket at its original scope i.e. reducing the number of exceptions FieldFacetAccumulator throws (perhaps leaving the IOException and removing the SolrException as per our comments above) and for the check moving efforts we'd create a separate JIRA ticket. How does that sound?

        Show
        Christine Poerschke added a comment - Okay, so if we're looking at moving not only hasDocValues() but also other error checking in a similar manner out of the accumulator class(es) - how about we keep this JIRA ticket at its original scope i.e. reducing the number of exceptions FieldFacetAccumulator throws (perhaps leaving the IOException and removing the SolrException as per our comments above) and for the check moving efforts we'd create a separate JIRA ticket. How does that sound?
        Hide
        Christine Poerschke added a comment -

        Happy to take this JIRA and apply/commit patch.

        Scott Stults - let me know if you'd like to go with the original patch or if you'd prefer to attach a revised patch and go with IOException instead of SolrException instead based on our comments above. Thank you.

        Show
        Christine Poerschke added a comment - Happy to take this JIRA and apply/commit patch. Scott Stults - let me know if you'd like to go with the original patch or if you'd prefer to attach a revised patch and go with IOException instead of SolrException instead based on our comments above. Thank you.
        Hide
        Scott Stults added a comment -

        This patch is like the last one except it's using IOException rather than SolrException. And yeah, let's keep this Jira short and sweet and open a separate one for moving the checks.

        Show
        Scott Stults added a comment - This patch is like the last one except it's using IOException rather than SolrException. And yeah, let's keep this Jira short and sweet and open a separate one for moving the checks.
        Hide
        ASF subversion and git services added a comment -

        Commit 1712751 from Christine Poerschke in branch 'dev/trunk'
        [ https://svn.apache.org/r1712751 ]

        SOLR-8147: contrib/analytics FieldFacetAccumulator now throws IOException instead of SolrException

        Show
        ASF subversion and git services added a comment - Commit 1712751 from Christine Poerschke in branch 'dev/trunk' [ https://svn.apache.org/r1712751 ] SOLR-8147 : contrib/analytics FieldFacetAccumulator now throws IOException instead of SolrException
        Hide
        ASF subversion and git services added a comment -

        Commit 1712783 from Christine Poerschke in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1712783 ]

        SOLR-8147: contrib/analytics FieldFacetAccumulator now throws IOException instead of SolrException (merge in revision 1712751 from trunk)

        Show
        ASF subversion and git services added a comment - Commit 1712783 from Christine Poerschke in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1712783 ] SOLR-8147 : contrib/analytics FieldFacetAccumulator now throws IOException instead of SolrException (merge in revision 1712751 from trunk)
        Hide
        Christine Poerschke added a comment -

        SOLR-8242 'contrib/analytics: relocate hasDocValues() check out of FieldFacetAccumulator' created for moving the checks.

        Show
        Christine Poerschke added a comment - SOLR-8242 'contrib/analytics: relocate hasDocValues() check out of FieldFacetAccumulator' created for moving the checks.
        Hide
        Christine Poerschke added a comment -

        Change committed to trunk and branch_5x (for 5.4). Thanks Scott!

        Show
        Christine Poerschke added a comment - Change committed to trunk and branch_5x (for 5.4). Thanks Scott!

          People

          • Assignee:
            Christine Poerschke
            Reporter:
            Scott Stults
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development