Solr
  1. Solr
  2. SOLR-5623

Better diagnosis of RuntimeExceptions in analysis

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.6.1
    • Fix Version/s: 4.7, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      If an analysis component (tokenizer, filter, etc) gets really into a hissy fit and throws a RuntimeException, the resulting log traffic is less than informative, lacking any pointer to the doc under discussion (in the doc case). It would be more better if there was a catch/try shortstop that logged this more informatively.

      1. SOLR-5623-nowrap.patch
        1 kB
        Shalin Shekhar Mangar
      2. SOLR-5623-nowrap.patch
        0.8 kB
        Shalin Shekhar Mangar

        Issue Links

          Activity

          Hide
          Benson Margulies added a comment -

          Chris Hostetter (Unused) https://github.com/apache/lucene-solr/pull/18 shows the failing test case.

          How do I make a test that asserts facts about logging? I can certainly use this to make some improvements to the logging, but I don't know how to automate proving that I did it?

          Show
          Benson Margulies added a comment - Chris Hostetter (Unused) https://github.com/apache/lucene-solr/pull/18 shows the failing test case. How do I make a test that asserts facts about logging? I can certainly use this to make some improvements to the logging, but I don't know how to automate proving that I did it?
          Hide
          Hoss Man added a comment -

          How do I make a test that asserts facts about logging?

          That's what i was mentioning in the email thread – i don't know of anyway to assert that something is logged, but you can assert that adding a document results in an exception being thrown to the client which matches a specified expression (which is probably the most important thing in this situation anyway) and then that should also result in the exception being logged.

          There's probably some way we could setup a MockLogListener or something in the test framework, and tell it what to pay attention to and assert that it sees those exceptions after the appropriate client calls – but we don't have anything like that and of the top of my head i don't know how to do it.

          Show
          Hoss Man added a comment - How do I make a test that asserts facts about logging? That's what i was mentioning in the email thread – i don't know of anyway to assert that something is logged, but you can assert that adding a document results in an exception being thrown to the client which matches a specified expression (which is probably the most important thing in this situation anyway) and then that should also result in the exception being logged. There's probably some way we could setup a MockLogListener or something in the test framework, and tell it what to pay attention to and assert that it sees those exceptions after the appropriate client calls – but we don't have anything like that and of the top of my head i don't know how to do it.
          Hide
          Benson Margulies added a comment -

          OK. Does it make sense to adopt the idea that 'if there is an ID field with a value, include that in the exception"?

          Show
          Benson Margulies added a comment - OK. Does it make sense to adopt the idea that 'if there is an ID field with a value, include that in the exception"?
          Hide
          Benson Margulies added a comment - - edited

          Robert Muir The identity of the field we are processing is known down in Lucene core. What do you think about wrapping Throwables in org.apache.lucene.index.DocInverterPerField.processFields in some specific runtime exception that carries the field name?

          Then I can in turn make it into a Solr exception in DirectUpdateHandler2.

          Show
          Benson Margulies added a comment - - edited Robert Muir The identity of the field we are processing is known down in Lucene core. What do you think about wrapping Throwables in org.apache.lucene.index.DocInverterPerField.processFields in some specific runtime exception that carries the field name? Then I can in turn make it into a Solr exception in DirectUpdateHandler2.
          Hide
          Robert Muir added a comment -

          Benson I don't know about that, its pretty tricky.

          Especially this particular place in the code, I think we should keep as simple as possible and not be messing with such exceptions. There is already plenty complexity here (aborting vs non-aborting exceptions) and so on for lucene to deal with.

          In general whats happening here is not happening inside indexwriter, its happening in the analysis chain. I think solr or other applications is the right place to add additional debugging information (such as a unique ID for the document) because only it has that additional context to ensure its what is useful to get to the bottom.

          Show
          Robert Muir added a comment - Benson I don't know about that, its pretty tricky. Especially this particular place in the code, I think we should keep as simple as possible and not be messing with such exceptions. There is already plenty complexity here (aborting vs non-aborting exceptions) and so on for lucene to deal with. In general whats happening here is not happening inside indexwriter, its happening in the analysis chain. I think solr or other applications is the right place to add additional debugging information (such as a unique ID for the document) because only it has that additional context to ensure its what is useful to get to the bottom.
          Hide
          Benson Margulies added a comment -

          OK, we can log and return the ID and not the field name, and that's already an improvement, so I'll stick with that.

          Show
          Benson Margulies added a comment - OK, we can log and return the ID and not the field name, and that's already an improvement, so I'll stick with that.
          Hide
          Robert Muir added a comment -

          I'm not completely opposed to it for the record, i'm just saying "i dont know about it". At the very least ill say, i'd be less opposed in trunk because the logic there it is simpler (due to java7 try-with: no boolean success/success2 heh). Still, as an API i dont like the fact that we'd be wrapping some specific exception with a bogus-generic one...

          Show
          Robert Muir added a comment - I'm not completely opposed to it for the record, i'm just saying "i dont know about it". At the very least ill say, i'd be less opposed in trunk because the logic there it is simpler (due to java7 try-with: no boolean success/success2 heh). Still, as an API i dont like the fact that we'd be wrapping some specific exception with a bogus-generic one...
          Hide
          Benson Margulies added a comment -

          Back to the exception decoration problem:

          There's a general design puzzle here: an outer function knows something that an inner function does not, and the catcher of the exception wants to know both. I share your distaste for the obvious Java solution of endless exception wrapping. Another option is to log, but does the Lucene code log things?

          I'm working against trunk because I don't know any better. I'm inclined to stay out at the Solr level for now, and maybe make another patch for this idea in the core later.

          Show
          Benson Margulies added a comment - Back to the exception decoration problem: There's a general design puzzle here: an outer function knows something that an inner function does not, and the catcher of the exception wants to know both. I share your distaste for the obvious Java solution of endless exception wrapping. Another option is to log, but does the Lucene code log things? I'm working against trunk because I don't know any better. I'm inclined to stay out at the Solr level for now, and maybe make another patch for this idea in the core later.
          Hide
          Benson Margulies added a comment - - edited

          Chris Hostetter (Unused) I think the patch request is now good to go, again sticking with a Solr change and considering a Lucene change later on.

          Show
          Benson Margulies added a comment - - edited Chris Hostetter (Unused) I think the patch request is now good to go, again sticking with a Solr change and considering a Lucene change later on.
          Hide
          Hoss Man added a comment -

          In general whats happening here is not happening inside indexwriter, its happening in the analysis chain. I think solr or other applications is the right place to add additional debugging information (such as a unique ID for the document) because only it has that additional context to ensure its what is useful to get to the bottom.

          Agreed ... but it would be nice if (in addition to application concepts like the uniqueKey of the doc) the exceptions could be annotated with information like what field name was associated with the runtime exception – I don't think there's currently anyway for code "above" IndexWriter to do that is there?

          The flip side though is that having this kind of logic in IndexWriter (or DocInverterPerField, or wherever under the covers) to wrap any arbitrary Runtime exception (maybe IllegalArgumentEx, maybe ArrayOutOfBounds, etc...) with some kind of generic LuceneAnalysisRuntimeException that contains a "getField" method seems like a really bad idea since it would hide (via wrapping) the true underlying exception type. We do this a lot in Solr since ultimately we're always going to need to propagate a SolrException with a status code to the remote client – but i don't think anything else in Lucene Core wraps exceptions like this.

          I don't know of any sane way to deal with this kind of problem – just pointing out that knowing the field name that caused the problem seems equally important to knowing the uniqueKey. (in case anybody else has any good ideas).

          In any case, we can make progress on the fairly easy part: annotating with the unqieuKey in Solr...

          Benson, comments on your current pull request:

          • there's some cut/paste comments/javadocs in the test configs/classes that need corrected
          • considering things like SOLR-4992, i don't think adding a "catch (Throwable t)" is a good idea ... i would constrain this to RuntimeException
          • take a look at AddUpdateCommand.getPrintableId
          • your try/catch/wrap block is only arround one code path that calls IndexWriter.updateDocument* ... there are others. The most straightforward/safe approach would probably be to refactor the entire addDoc(AddUpdateCommand) method along the lines of...
              public int addDoc(AddUpdateCommand cmd) throws IOException {
                try { 
                  return addDocInternal(cmd) 
                } catch (...) {
                   ...
                }
              }
              // nocommit: javadocs as to purpose
              private int addDocInternal(AddUpdateCommand cmd) throws IOException {
                ...
              }
            
          • this recipe is a bit cleaner for the type of assertion you are doing...
              try {
                doSomethingThatShouldThrowAndException();
                fail("didn't get expected exception");
              } catch (ExpectedExceptionType e) {
                assertStuffAbout(e);
              }
            
          Show
          Hoss Man added a comment - In general whats happening here is not happening inside indexwriter, its happening in the analysis chain. I think solr or other applications is the right place to add additional debugging information (such as a unique ID for the document) because only it has that additional context to ensure its what is useful to get to the bottom. Agreed ... but it would be nice if (in addition to application concepts like the uniqueKey of the doc) the exceptions could be annotated with information like what field name was associated with the runtime exception – I don't think there's currently anyway for code "above" IndexWriter to do that is there? The flip side though is that having this kind of logic in IndexWriter (or DocInverterPerField, or wherever under the covers) to wrap any arbitrary Runtime exception (maybe IllegalArgumentEx, maybe ArrayOutOfBounds, etc...) with some kind of generic LuceneAnalysisRuntimeException that contains a "getField" method seems like a really bad idea since it would hide (via wrapping) the true underlying exception type. We do this a lot in Solr since ultimately we're always going to need to propagate a SolrException with a status code to the remote client – but i don't think anything else in Lucene Core wraps exceptions like this. I don't know of any sane way to deal with this kind of problem – just pointing out that knowing the field name that caused the problem seems equally important to knowing the uniqueKey. (in case anybody else has any good ideas). In any case, we can make progress on the fairly easy part: annotating with the unqieuKey in Solr... Benson, comments on your current pull request: there's some cut/paste comments/javadocs in the test configs/classes that need corrected considering things like SOLR-4992 , i don't think adding a "catch (Throwable t)" is a good idea ... i would constrain this to RuntimeException take a look at AddUpdateCommand.getPrintableId your try/catch/wrap block is only arround one code path that calls IndexWriter.updateDocument* ... there are others. The most straightforward/safe approach would probably be to refactor the entire addDoc(AddUpdateCommand) method along the lines of... public int addDoc(AddUpdateCommand cmd) throws IOException { try { return addDocInternal(cmd) } catch (...) { ... } } // nocommit: javadocs as to purpose private int addDocInternal(AddUpdateCommand cmd) throws IOException { ... } this recipe is a bit cleaner for the type of assertion you are doing... try { doSomethingThatShouldThrowAndException(); fail( "didn't get expected exception" ); } catch (ExpectedExceptionType e) { assertStuffAbout(e); }
          Hide
          Benson Margulies added a comment -

          OK, pushed changes as per remarks.

          Show
          Benson Margulies added a comment - OK, pushed changes as per remarks.
          Hide
          Benson Margulies added a comment -

          Chris Hostetter (Unused) have you looked at my revs?

          Show
          Benson Margulies added a comment - Chris Hostetter (Unused) have you looked at my revs?
          Hide
          Hoss Man added a comment -

          +1

          Show
          Hoss Man added a comment - +1
          Hide
          ASF subversion and git services added a comment -

          Commit 1564584 from Benson Margulies in branch 'dev/trunk'
          [ https://svn.apache.org/r1564584 ]

          SOLR-5623: Better diagnosis of RuntimeExceptions in analysis

          Show
          ASF subversion and git services added a comment - Commit 1564584 from Benson Margulies in branch 'dev/trunk' [ https://svn.apache.org/r1564584 ] SOLR-5623 : Better diagnosis of RuntimeExceptions in analysis
          Hide
          Benson Margulies added a comment -

          trunk patch 1564584.

          Show
          Benson Margulies added a comment - trunk patch 1564584.
          Hide
          ASF subversion and git services added a comment -

          Commit 1564592 from Benson Margulies in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1564592 ]

          SOLR-5623: backport and fill in CHANGES.

          Show
          ASF subversion and git services added a comment - Commit 1564592 from Benson Margulies in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1564592 ] SOLR-5623 : backport and fill in CHANGES.
          Hide
          Benson Margulies added a comment -

          backported in 1564592.

          Show
          Benson Margulies added a comment - backported in 1564592.
          Hide
          Shalin Shekhar Mangar added a comment -

          The change to DirectUpdateHandler2 broke some tests because now it wraps a SolrException again in another SolrException and some tests were expecting the exception's cause to be the real cause. The attached patch only wraps the RuntimeException if they're not SolrException. Thoughts?

          Show
          Shalin Shekhar Mangar added a comment - The change to DirectUpdateHandler2 broke some tests because now it wraps a SolrException again in another SolrException and some tests were expecting the exception's cause to be the real cause. The attached patch only wraps the RuntimeException if they're not SolrException. Thoughts?
          Hide
          Shalin Shekhar Mangar added a comment -

          Fixes forbidden-api-check failure on String.format.

          [forbidden-apis] Scanning for API signatures and dependencies...
          [forbidden-apis] Forbidden method invocation: java.lang.String#format(java.lang.String,java.lang.Object[]) [Uses default locale]
          [forbidden-apis]   in org.apache.solr.update.DirectUpdateHandler2 (DirectUpdateHandler2.java:162)
          
          Show
          Shalin Shekhar Mangar added a comment - Fixes forbidden-api-check failure on String.format. [forbidden-apis] Scanning for API signatures and dependencies... [forbidden-apis] Forbidden method invocation: java.lang. String #format(java.lang. String ,java.lang. Object []) [Uses default locale] [forbidden-apis] in org.apache.solr.update.DirectUpdateHandler2 (DirectUpdateHandler2.java:162)
          Hide
          ASF subversion and git services added a comment -

          Commit 1564700 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1564700 ]

          SOLR-5623: Use root locale in String.format and do not wrap SolrExceptions

          Show
          ASF subversion and git services added a comment - Commit 1564700 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1564700 ] SOLR-5623 : Use root locale in String.format and do not wrap SolrExceptions
          Hide
          ASF subversion and git services added a comment -

          Commit 1564701 from shalin@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1564701 ]

          SOLR-5623: Use root locale in String.format and do not wrap SolrExceptions

          Show
          ASF subversion and git services added a comment - Commit 1564701 from shalin@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1564701 ] SOLR-5623 : Use root locale in String.format and do not wrap SolrExceptions
          Hide
          ASF subversion and git services added a comment -

          Commit 1564737 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1564737 ]

          SOLR-5623: Added svn:eol-style

          Show
          ASF subversion and git services added a comment - Commit 1564737 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1564737 ] SOLR-5623 : Added svn:eol-style
          Hide
          ASF subversion and git services added a comment -

          Commit 1564741 from shalin@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1564741 ]

          SOLR-5623: Added svn:eol-style

          Show
          ASF subversion and git services added a comment - Commit 1564741 from shalin@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1564741 ] SOLR-5623 : Added svn:eol-style
          Hide
          Benson Margulies added a comment -

          Shalin Shekhar Mangar Apparently I haven't learned to read the output of ant test very well, and fooled myself into believing that all as well. Thanks for cleaning up after me.

          Show
          Benson Margulies added a comment - Shalin Shekhar Mangar Apparently I haven't learned to read the output of ant test very well, and fooled myself into believing that all as well. Thanks for cleaning up after me.
          Hide
          Shalin Shekhar Mangar added a comment -

          No problem, it happens to all of us. I've been guilty of it more often than others I think. Running the test suite is not enough, you need 'ant precommit' to pass as well.

          Show
          Shalin Shekhar Mangar added a comment - No problem, it happens to all of us. I've been guilty of it more often than others I think. Running the test suite is not enough, you need 'ant precommit' to pass as well.
          Hide
          ASF subversion and git services added a comment -

          Commit 1564831 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1564831 ]

          SOLR-5623: revert r1564739, shalin already fixed the bug that caused these failures, but Uwe didn't know that

          Show
          ASF subversion and git services added a comment - Commit 1564831 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1564831 ] SOLR-5623 : revert r1564739, shalin already fixed the bug that caused these failures, but Uwe didn't know that
          Hide
          ASF subversion and git services added a comment -

          Commit 1564834 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1564834 ]

          SOLR-5623: revert r1564739, shalin already fixed the bug that caused these failures, but Uwe didn't know that (merge r1564831)

          Show
          ASF subversion and git services added a comment - Commit 1564834 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1564834 ] SOLR-5623 : revert r1564739, shalin already fixed the bug that caused these failures, but Uwe didn't know that (merge r1564831)

            People

            • Assignee:
              Benson Margulies
              Reporter:
              Benson Margulies
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development