`TestIndexWriterOnDiskFull.testAddIndexOnDiskFull` reproducible test failure about lucene HOT 11 CLOSED

This is indeed a test bug, 9ab84f4 just slightly changes the index size. There was a similar issue: #11755, the UncheckedIOException might be throw by BKDWriter but not catch in this test case. This should likely be fixed e.g.

@@ -364,7 +365,7 @@ public class TestIndexWriterOnDiskFull extends LuceneTestCase {
               done = true;
             }

-          } catch (IllegalStateException | IOException | MergePolicy.MergeException e) {
+          } catch (IllegalStateException | IOException | MergePolicy.MergeException | UncheckedIOException e) {
             success = false;
             err = e;
             if (VERBOSE) {

The testAddDocumentOnDiskFull should also have the same issue.

from lucene.

A much better solution would be to try to track down why UncheckedIOException is thrown in the stack - something is likely missing a proper IOException signature (or something higher up the stack should catch uncheckedIOE and rethrow the original cause?).

Those unchecked exceptions are hell. If they can indeed happen under certain circumstances, it would be better to handle these cases as plan (predictable) IOE. At least that's my opinion - the discussion concerning checked/ unchecked exceptions is as old as Java (my opinion being it was a design mistake to introduce checked exceptions; it's particularly brutal when you're trying to use lambdas with I/O).

from lucene.

This was on Java 11:

raptorlake:99x[branch_9_10]$ java -fullversion
openjdk full version "11.0.21+9"

from lucene.

bisect shows it related to : 9ab84f4

from lucene.

Oooh thanks for digging @easyice!

Hmm, that may just be a seed-shifting change, and not e.g. bringing the root cause for this exception? I.e. 9ab84f4 just changed the interpretation of the random seed ("seed shifting").

The exception looks like a test bug to me: disk full is arriving to a merge thread (ConcurrentMergeScheduler), which is expected (this is a disk full test), so maybe the test is failing to properly recognize that this is "normal" for it?

from lucene.

Thanks for explaining @mikemccand , I haven't fully understand the related between this exception and 9ab84f4, I agree with you they may not have a direct related.

In general, the "fake disk full" exception may be due to the absence of some exception type in the catch, such as UncheckedIOException , But if revert 9ab84f4 it will doesn't have this exception.

from lucene.

I like that proposed solution @easyice! Exception handling is hard. Likely many Lucene tests are missing/failing to catch UncheckedIOException now...

from lucene.

Likely many Lucene tests are missing/failing to catch UncheckedIOException now...

emmm... yeah, should we open another issue about this?

from lucene.

In BKDWriter, it seems we not pass a class that implements Runnable into the Thread framework, it just call run method directly, so maybe we can consider introducing CheckedRunnable to replace Runnable, like:

@FunctionalInterface
public interface CheckedRunnable<E extends Exception> {
  void run() throws E;
}

In this way, IOException can be thrown in CheckedRunnable.

from lucene.

I am not familiar with this code but there are many options to choose from. A Callable (this throws an Exception), Lucene's IOConsumer, adding an IORunnable similar to IOConsumer, IOFunction or IOSupplier we already have.. I would opt for an IORunnable or IOCallable, which could look like this:

interface IOCallable<T> extends Callable<T> {
      @Override
      public T call() throws IOException;
  }

narrowing the type of exceptions thrown to only IOException and keeping an option to return something from the inside, at the same time allowing submission of such blocks to plain Java concurrent infrastructure? Just a thought.

from lucene.

This is also a good idea! If we use CheckedRunnable, we also need to narrow the type of exceptions thrown to IOException only. like:

- Runnable finalizer = writer.writeField(...)
+ CheckedRunnable<IOException> finalizer = writer.writeField(...)

IORunnable will makes the code more concise, the use of Runnable outside of the thread framework seems not widespread.

@mikemccand , what do you think of this way of avoiding throwing an UncheckedIOException rather than catching it?

from lucene.