Mailing List Archive

Re: [lucene] branch main updated: LUCENE-10088: allow per-class override in HandleLimitFS. Bump the limit a bit for nightlies in TestIndexWriterMergePolicy. (#424)
Hopefully we can find a better long-term fix for this test?
If fixing IndexWriter throttling is too tricky, maybe we could simply
improve the test with a short-term fix to use less handles (e.g. force
NRTCachingDir or RAMDir)

2048 is already way too high, the default OS limit is 1024 on every
linux box i've seen. On the MacOS I remember a time when it was very
low, but I'm seeing a limit of 2560 on 10.15 (Catalina). I have no
clue what limits might be present on Windows.

The purpose of the "artificial" limit via nio.2 is to make such
failures more reproducible across different machines. But if it is
cranked high, then it won't serve its purpose very well.

On Thu, Nov 4, 2021 at 3:32 AM <dweiss@apache.org> wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> dweiss pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/lucene.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new adec73d LUCENE-10088: allow per-class override in HandleLimitFS. Bump the limit a bit for nightlies in TestIndexWriterMergePolicy. (#424)
> adec73d is described below
>
> commit adec73dd284ebf565d7d205cccc519dac1ffe61c
> Author: Dawid Weiss <dawid.weiss@carrotsearch.com>
> AuthorDate: Thu Nov 4 08:31:28 2021 +0100
>
> LUCENE-10088: allow per-class override in HandleLimitFS. Bump the limit a bit for nightlies in TestIndexWriterMergePolicy. (#424)
> ---
> .../lucene/index/TestIndexWriterMergePolicy.java | 8 +++++---
> .../org/apache/lucene/mockfile/HandleLimitFS.java | 24 +++++++++++++++++++++-
> .../lucene/util/TestRuleTemporaryFilesCleanup.java | 10 ++++-----
> 3 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> index 2578712..6192e0e 100644
> --- a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> +++ b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> @@ -28,12 +28,14 @@ import org.apache.lucene.document.Field;
> import org.apache.lucene.document.NumericDocValuesField;
> import org.apache.lucene.document.StringField;
> import org.apache.lucene.index.IndexWriterConfig.OpenMode;
> +import org.apache.lucene.mockfile.HandleLimitFS;
> import org.apache.lucene.search.IndexSearcher;
> import org.apache.lucene.search.MatchAllDocsQuery;
> import org.apache.lucene.search.TermQuery;
> import org.apache.lucene.store.Directory;
> import org.apache.lucene.util.LuceneTestCase;
>
> +@HandleLimitFS.MaxOpenHandles(limit = HandleLimitFS.MaxOpenHandles.MAX_OPEN_FILES * 2)
> public class TestIndexWriterMergePolicy extends LuceneTestCase {
>
> // Test the normal case
> @@ -772,9 +774,9 @@ public class TestIndexWriterMergePolicy extends LuceneTestCase {
> if (random().nextBoolean()) {
> writer.commit();
> }
> - try (DirectoryReader open =
> - new SoftDeletesDirectoryReaderWrapper(
> - DirectoryReader.open(directory), "___soft_deletes")) {
> + try (DirectoryReader delegate = DirectoryReader.open(directory);
> + DirectoryReader open =
> + new SoftDeletesDirectoryReaderWrapper(delegate, "___soft_deletes")) {
> assertEquals(
> 1,
> new IndexSearcher(open)
> diff --git a/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java b/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> index 393c1de..ece3ba2 100644
> --- a/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> +++ b/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> @@ -17,16 +17,38 @@
> package org.apache.lucene.mockfile;
>
> import java.io.IOException;
> +import java.lang.annotation.Documented;
> +import java.lang.annotation.ElementType;
> +import java.lang.annotation.Inherited;
> +import java.lang.annotation.Retention;
> +import java.lang.annotation.RetentionPolicy;
> +import java.lang.annotation.Target;
> import java.nio.file.FileSystem;
> import java.nio.file.FileSystemException;
> import java.nio.file.Path;
> import java.util.concurrent.atomic.AtomicInteger;
>
> -/** FileSystem that throws exception if file handles in use exceeds a specified limit */
> +/**
> + * FileSystem that throws exception if file handles in use exceeds a specified limit.
> + *
> + * @see MaxOpenHandles
> + */
> public class HandleLimitFS extends HandleTrackingFS {
> final int limit;
> final AtomicInteger count = new AtomicInteger();
>
> + /** An annotation */
> + @Documented
> + @Inherited
> + @Retention(RetentionPolicy.RUNTIME)
> + @Target(ElementType.TYPE)
> + public static @interface MaxOpenHandles {
> + // TODO: can we make the default even lower?
> + public static final int MAX_OPEN_FILES = 2048;
> +
> + int limit();
> + }
> +
> /**
> * Create a new instance, limiting the maximum number of open files to {@code limit}
> *
> diff --git a/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java b/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> index 3b0b153..6c67efd 100644
> --- a/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> +++ b/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> @@ -100,10 +100,6 @@ final class TestRuleTemporaryFilesCleanup extends TestRuleAdapter {
> javaTempDir = initializeJavaTempDir();
> }
>
> - // os/config-independent limit for too many open files
> - // TODO: can we make this lower?
> - private static final int MAX_OPEN_FILES = 2048;
> -
> private boolean allowed(Set<String> avoid, Class<? extends FileSystemProvider> clazz) {
> if (avoid.contains("*") || avoid.contains(clazz.getSimpleName())) {
> return false;
> @@ -152,7 +148,11 @@ final class TestRuleTemporaryFilesCleanup extends TestRuleAdapter {
> fs = new LeakFS(fs).getFileSystem(null);
> }
> if (allowed(avoid, HandleLimitFS.class)) {
> - fs = new HandleLimitFS(fs, MAX_OPEN_FILES).getFileSystem(null);
> + int limit = HandleLimitFS.MaxOpenHandles.MAX_OPEN_FILES;
> + if (targetClass.isAnnotationPresent(HandleLimitFS.MaxOpenHandles.class)) {
> + limit = targetClass.getAnnotation(HandleLimitFS.MaxOpenHandles.class).limit();
> + }
> + fs = new HandleLimitFS(fs, limit).getFileSystem(null);
> }
> // windows is currently slow
> if (random.nextInt(10) == 0) {

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [lucene] branch main updated: LUCENE-10088: allow per-class override in HandleLimitFS. Bump the limit a bit for nightlies in TestIndexWriterMergePolicy. (#424) [ In reply to ]
I think there should be a better solution based on throttling. This test
runs with 4 threads - I can imagine processes where an order of magnitude
more threads will index in parallel (+ merges). So it should just work. I
don't know how to fix it though it's causing additional noise for me when I
scan the mailing list. Isn't switching to a different directory going to be
a similar kind of non-fix workaround?

D.

On Thu, Nov 4, 2021 at 12:48 PM Robert Muir <rcmuir@gmail.com> wrote:

> Hopefully we can find a better long-term fix for this test?
> If fixing IndexWriter throttling is too tricky, maybe we could simply
> improve the test with a short-term fix to use less handles (e.g. force
> NRTCachingDir or RAMDir)
>
> 2048 is already way too high, the default OS limit is 1024 on every
> linux box i've seen. On the MacOS I remember a time when it was very
> low, but I'm seeing a limit of 2560 on 10.15 (Catalina). I have no
> clue what limits might be present on Windows.
>
> The purpose of the "artificial" limit via nio.2 is to make such
> failures more reproducible across different machines. But if it is
> cranked high, then it won't serve its purpose very well.
>
> On Thu, Nov 4, 2021 at 3:32 AM <dweiss@apache.org> wrote:
> >
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > dweiss pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/lucene.git
> >
> >
> > The following commit(s) were added to refs/heads/main by this push:
> > new adec73d LUCENE-10088: allow per-class override in
> HandleLimitFS. Bump the limit a bit for nightlies in
> TestIndexWriterMergePolicy. (#424)
> > adec73d is described below
> >
> > commit adec73dd284ebf565d7d205cccc519dac1ffe61c
> > Author: Dawid Weiss <dawid.weiss@carrotsearch.com>
> > AuthorDate: Thu Nov 4 08:31:28 2021 +0100
> >
> > LUCENE-10088: allow per-class override in HandleLimitFS. Bump the
> limit a bit for nightlies in TestIndexWriterMergePolicy. (#424)
> > ---
> > .../lucene/index/TestIndexWriterMergePolicy.java | 8 +++++---
> > .../org/apache/lucene/mockfile/HandleLimitFS.java | 24
> +++++++++++++++++++++-
> > .../lucene/util/TestRuleTemporaryFilesCleanup.java | 10 ++++-----
> > 3 files changed, 33 insertions(+), 9 deletions(-)
> >
> > diff --git
> a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> > index 2578712..6192e0e 100644
> > ---
> a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> > +++
> b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMergePolicy.java
> > @@ -28,12 +28,14 @@ import org.apache.lucene.document.Field;
> > import org.apache.lucene.document.NumericDocValuesField;
> > import org.apache.lucene.document.StringField;
> > import org.apache.lucene.index.IndexWriterConfig.OpenMode;
> > +import org.apache.lucene.mockfile.HandleLimitFS;
> > import org.apache.lucene.search.IndexSearcher;
> > import org.apache.lucene.search.MatchAllDocsQuery;
> > import org.apache.lucene.search.TermQuery;
> > import org.apache.lucene.store.Directory;
> > import org.apache.lucene.util.LuceneTestCase;
> >
> > +@HandleLimitFS.MaxOpenHandles(limit =
> HandleLimitFS.MaxOpenHandles.MAX_OPEN_FILES * 2)
> > public class TestIndexWriterMergePolicy extends LuceneTestCase {
> >
> > // Test the normal case
> > @@ -772,9 +774,9 @@ public class TestIndexWriterMergePolicy extends
> LuceneTestCase {
> > if (random().nextBoolean()) {
> > writer.commit();
> > }
> > - try (DirectoryReader open =
> > - new SoftDeletesDirectoryReaderWrapper(
> > - DirectoryReader.open(directory),
> "___soft_deletes")) {
> > + try (DirectoryReader delegate =
> DirectoryReader.open(directory);
> > + DirectoryReader open =
> > + new SoftDeletesDirectoryReaderWrapper(delegate,
> "___soft_deletes")) {
> > assertEquals(
> > 1,
> > new IndexSearcher(open)
> > diff --git
> a/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> b/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> > index 393c1de..ece3ba2 100644
> > ---
> a/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> > +++
> b/lucene/test-framework/src/java/org/apache/lucene/mockfile/HandleLimitFS.java
> > @@ -17,16 +17,38 @@
> > package org.apache.lucene.mockfile;
> >
> > import java.io.IOException;
> > +import java.lang.annotation.Documented;
> > +import java.lang.annotation.ElementType;
> > +import java.lang.annotation.Inherited;
> > +import java.lang.annotation.Retention;
> > +import java.lang.annotation.RetentionPolicy;
> > +import java.lang.annotation.Target;
> > import java.nio.file.FileSystem;
> > import java.nio.file.FileSystemException;
> > import java.nio.file.Path;
> > import java.util.concurrent.atomic.AtomicInteger;
> >
> > -/** FileSystem that throws exception if file handles in use exceeds a
> specified limit */
> > +/**
> > + * FileSystem that throws exception if file handles in use exceeds a
> specified limit.
> > + *
> > + * @see MaxOpenHandles
> > + */
> > public class HandleLimitFS extends HandleTrackingFS {
> > final int limit;
> > final AtomicInteger count = new AtomicInteger();
> >
> > + /** An annotation */
> > + @Documented
> > + @Inherited
> > + @Retention(RetentionPolicy.RUNTIME)
> > + @Target(ElementType.TYPE)
> > + public static @interface MaxOpenHandles {
> > + // TODO: can we make the default even lower?
> > + public static final int MAX_OPEN_FILES = 2048;
> > +
> > + int limit();
> > + }
> > +
> > /**
> > * Create a new instance, limiting the maximum number of open files
> to {@code limit}
> > *
> > diff --git
> a/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> b/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> > index 3b0b153..6c67efd 100644
> > ---
> a/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> > +++
> b/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleTemporaryFilesCleanup.java
> > @@ -100,10 +100,6 @@ final class TestRuleTemporaryFilesCleanup extends
> TestRuleAdapter {
> > javaTempDir = initializeJavaTempDir();
> > }
> >
> > - // os/config-independent limit for too many open files
> > - // TODO: can we make this lower?
> > - private static final int MAX_OPEN_FILES = 2048;
> > -
> > private boolean allowed(Set<String> avoid, Class<? extends
> FileSystemProvider> clazz) {
> > if (avoid.contains("*") || avoid.contains(clazz.getSimpleName())) {
> > return false;
> > @@ -152,7 +148,11 @@ final class TestRuleTemporaryFilesCleanup extends
> TestRuleAdapter {
> > fs = new LeakFS(fs).getFileSystem(null);
> > }
> > if (allowed(avoid, HandleLimitFS.class)) {
> > - fs = new HandleLimitFS(fs, MAX_OPEN_FILES).getFileSystem(null);
> > + int limit = HandleLimitFS.MaxOpenHandles.MAX_OPEN_FILES;
> > + if
> (targetClass.isAnnotationPresent(HandleLimitFS.MaxOpenHandles.class)) {
> > + limit =
> targetClass.getAnnotation(HandleLimitFS.MaxOpenHandles.class).limit();
> > + }
> > + fs = new HandleLimitFS(fs, limit).getFileSystem(null);
> > }
> > // windows is currently slow
> > if (random.nextInt(10) == 0) {
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: [lucene] branch main updated: LUCENE-10088: allow per-class override in HandleLimitFS. Bump the limit a bit for nightlies in TestIndexWriterMergePolicy. (#424) [ In reply to ]
On Thu, Nov 4, 2021 at 9:17 AM Dawid Weiss <dawid.weiss@gmail.com> wrote:
>
> I think there should be a better solution based on throttling. This test runs with 4 threads - I can imagine processes where an order of magnitude more threads will index in parallel (+ merges). So it should just work. I don't know how to fix it though it's causing additional noise for me when I scan the mailing list. Isn't switching to a different directory going to be a similar kind of non-fix workaround?
>

I don't think of it as a non-fix workaround. the test is supposed to
be testing merge-on-getreader right? It isn't supposed to be testing
throttling of excessive filehandles, although it is nice that it
"discovered" an issue there.

IMO, ideally we would add a separate test for that that explicitly
tries to max out the file handles and ensures IW does the right thing.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [lucene] branch main updated: LUCENE-10088: allow per-class override in HandleLimitFS. Bump the limit a bit for nightlies in TestIndexWriterMergePolicy. (#424) [ In reply to ]
Fair enough. That test has more problems as the nightly worst-case is just
a total killer time-wise... I really think it should have a wall-clock
expiration deadline inside too.

Dawid

On Thu, Nov 4, 2021 at 2:26 PM Robert Muir <rcmuir@gmail.com> wrote:

> On Thu, Nov 4, 2021 at 9:17 AM Dawid Weiss <dawid.weiss@gmail.com> wrote:
> >
> > I think there should be a better solution based on throttling. This test
> runs with 4 threads - I can imagine processes where an order of magnitude
> more threads will index in parallel (+ merges). So it should just work. I
> don't know how to fix it though it's causing additional noise for me when I
> scan the mailing list. Isn't switching to a different directory going to be
> a similar kind of non-fix workaround?
> >
>
> I don't think of it as a non-fix workaround. the test is supposed to
> be testing merge-on-getreader right? It isn't supposed to be testing
> throttling of excessive filehandles, although it is nice that it
> "discovered" an issue there.
>
> IMO, ideally we would add a separate test for that that explicitly
> tries to max out the file handles and ensures IW does the right thing.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>