Details

    • Lucene Fields:
      New

      Description

      functionally split large blocks of code that do logically different things, organize and order them into rules. Use RandomizedRunner facilities for ensuring scope resources (Directory, temporary folders) are properly disposed of. Use RandomizedRunner facilities for reporting reproduce message.

      I have a working branch for this at github. Polishing off the rough edges.

      1. LUCENE-4047.patch
        168 kB
        Dawid Weiss

        Issue Links

          Activity

          Hide
          Dawid Weiss added a comment -

          Incorporated in refactoring.

          Show
          Dawid Weiss added a comment - Incorporated in refactoring.
          Hide
          Dawid Weiss added a comment -

          A patch with cleanups to LuceneTestCase. Most of the previous code blobs from before-after methods have been refactored into rules that have a single concern (there are exceptions).

          I'd appreciate if folks could test against temporarily introduced bugs to check that the output indeed catches them and is consistent. It can be more verbose than previously (that is: more errors can be printed than before) because with rules you can push more than a single failure.

          Show
          Dawid Weiss added a comment - A patch with cleanups to LuceneTestCase. Most of the previous code blobs from before-after methods have been refactored into rules that have a single concern (there are exceptions). I'd appreciate if folks could test against temporarily introduced bugs to check that the output indeed catches them and is consistent. It can be more verbose than previously (that is: more errors can be printed than before) because with rules you can push more than a single failure.
          Hide
          Dawid Weiss added a comment -

          A patch with cleanups to LuceneTestCase. Most of the previous code blobs from before-after methods have been refactored into rules that have a single concern (there are exceptions).

          I'd appreciate if folks could test against temporarily introduced bugs to check that the output indeed catches them and is consistent. It can be more verbose than previously (that is: more errors can be printed than before) because with rules you can push more than a single failure.

          Show
          Dawid Weiss added a comment - A patch with cleanups to LuceneTestCase. Most of the previous code blobs from before-after methods have been refactored into rules that have a single concern (there are exceptions). I'd appreciate if folks could test against temporarily introduced bugs to check that the output indeed catches them and is consistent. It can be more verbose than previously (that is: more errors can be printed than before) because with rules you can push more than a single failure.
          Hide
          Dawid Weiss added a comment -

          Apologies for the noise – jira issues (very long lags) and apparently no cookie token for submitting forms.

          Show
          Dawid Weiss added a comment - Apologies for the noise – jira issues (very long lags) and apparently no cookie token for submitting forms.
          Hide
          Robert Muir added a comment -

          The DirectoryCloseable won't work: MockDirectoryWrapper checks for index corruption on close(),
          we cannot just silently ignore Throwable from here.

          Show
          Robert Muir added a comment - The DirectoryCloseable won't work: MockDirectoryWrapper checks for index corruption on close(), we cannot just silently ignore Throwable from here.
          Hide
          Robert Muir added a comment -

          At a glance, the rest of the patch looks great! I think just whenever we close an MDW, we should not be hiding any exceptions it throws, worst case we should wrap in runtime exception.

          Show
          Robert Muir added a comment - At a glance, the rest of the patch looks great! I think just whenever we close an MDW, we should not be hiding any exceptions it throws, worst case we should wrap in runtime exception.
          Hide
          Dawid Weiss added a comment -

          The DirectoryCloseable won't work: MockDirectoryWrapper checks for index corruption on close(),

          we cannot just silently ignore Throwable from here

          This only happens if the directory is not closed – it should always be closed so appropriate exception would be thrown at that time. I assume the "unclosed directory" exception indicates more serious flaw in the test and this should be fixed first?

          We can change it to propagate both unclosed and any potential other exception but this will only increase the amount of logs to be reviewed.

          Also note that prior to this patch directories were not closed at all if anything failed –

                if (!testsFailed) {
                  checkResourcesAfterClass(); 
          

          I think this is already better in that resources are always closed, regardless of any previous errors?

          Show
          Dawid Weiss added a comment - The DirectoryCloseable won't work: MockDirectoryWrapper checks for index corruption on close(), we cannot just silently ignore Throwable from here This only happens if the directory is not closed – it should always be closed so appropriate exception would be thrown at that time. I assume the "unclosed directory" exception indicates more serious flaw in the test and this should be fixed first? We can change it to propagate both unclosed and any potential other exception but this will only increase the amount of logs to be reviewed. Also note that prior to this patch directories were not closed at all if anything failed – if (!testsFailed) { checkResourcesAfterClass(); I think this is already better in that resources are always closed, regardless of any previous errors?
          Hide
          Robert Muir added a comment -

          I assume the "unclosed directory" exception indicates more serious flaw in the test and this should be fixed first?

          I don't think thats the case here. I guess its unclear to me if this close-hide-throwable is only used
          when the test is already going to fail anyway, and I see your point that today no closing happens, but
          from my POV i just think we should not mask any exceptions coming from MDW.close().

          If MDW.close() throws an exception because checkindex failed, this is really serious, and I don't
          care what happened elsewhere in the test.

          Show
          Robert Muir added a comment - I assume the "unclosed directory" exception indicates more serious flaw in the test and this should be fixed first? I don't think thats the case here. I guess its unclear to me if this close-hide-throwable is only used when the test is already going to fail anyway, and I see your point that today no closing happens, but from my POV i just think we should not mask any exceptions coming from MDW.close(). If MDW.close() throws an exception because checkindex failed, this is really serious, and I don't care what happened elsewhere in the test.
          Hide
          Robert Muir added a comment -

          And if you start to close MDW when tests have already failed, I'm not sure this
          will provide a lot of benefits because MDW itself is really nothing at all but
          a wrapper.

          Chances are when a test has failed, closing the MDW will just immediately throw
          an exception and not really accomplish anything, because MDW close() throws exception
          if there are still open files in the directory it wraps. So nothing will really get
          closed.

          Show
          Robert Muir added a comment - And if you start to close MDW when tests have already failed, I'm not sure this will provide a lot of benefits because MDW itself is really nothing at all but a wrapper. Chances are when a test has failed, closing the MDW will just immediately throw an exception and not really accomplish anything, because MDW close() throws exception if there are still open files in the directory it wraps. So nothing will really get closed.
          Hide
          Dawid Weiss added a comment -

          I had private communication with Robert while Jira was offline. I'll reiterate with a new patch that will preserve the previous behavior – that is: no close() calls if any of the tests failed.

          Show
          Dawid Weiss added a comment - I had private communication with Robert while Jira was offline. I'll reiterate with a new patch that will preserve the previous behavior – that is: no close() calls if any of the tests failed.
          Hide
          Robert Muir added a comment -

          Summary of concerns:

          • if we close the mdw and ignore exceptions, its not any better than today as in general the MDW will still have files open against it and will simply throw an exception and not actually close anything.
          • if we don't fix MDW etc to actually close (even when ugly stuff happens), then failed tests will cause leaks (imagine RAM directories piling up in the JVM)

          So my opinion is, since the rest of this patch is a nice cleanup, we should commit it without the MDW.close() change... for now. We should separately open a issue to fix MDW and anything like it to always actually close() its delegate in a finally block, ensuring it doesn't lose any real exceptions from any of its openfiles/checkindex/crash/unrefed fields checks.

          Show
          Robert Muir added a comment - Summary of concerns: if we close the mdw and ignore exceptions, its not any better than today as in general the MDW will still have files open against it and will simply throw an exception and not actually close anything. if we don't fix MDW etc to actually close (even when ugly stuff happens), then failed tests will cause leaks (imagine RAM directories piling up in the JVM) So my opinion is, since the rest of this patch is a nice cleanup, we should commit it without the MDW.close() change... for now. We should separately open a issue to fix MDW and anything like it to always actually close() its delegate in a finally block, ensuring it doesn't lose any real exceptions from any of its openfiles/checkindex/crash/unrefed fields checks.
          Hide
          Robert Muir added a comment -

          I opened LUCENE-4058 as a followup task to clean this up. We should investigate any other mock-wrappers and fix them the same way.

          Show
          Robert Muir added a comment - I opened LUCENE-4058 as a followup task to clean this up. We should investigate any other mock-wrappers and fix them the same way.
          Hide
          Dawid Weiss added a comment -

          Ok, so I'll commit this patch in as-is and then we'll fix things from there on?

          Show
          Dawid Weiss added a comment - Ok, so I'll commit this patch in as-is and then we'll fix things from there on?

            People

            • Assignee:
              Dawid Weiss
              Reporter:
              Dawid Weiss
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development