PrimaryNode: add configurable timeout to waitForAllRemotesToClose

[stevenschlansker]

Adds a configurable timeout for PrimaryNode waiting for remotes to close.
The default matches existing behavior. In the long run, maybe this wait loop goes away entirely, but I elected to make the most compatible change first.

Fixes #11674

[zhaih]

Thanks for making the change, overall LGTM, just have some small comments.

Also please don't forget to add an entry in CHANGES.txt

[mikemccand]

This makes total sense -- tying Primary to the behavior of Replicas is dangerous coupling. It would be great to fix/improve the design so that Primary never has to wait for Replicas to do anything, but let's start with this great baby step.

[stevenschlansker]

I updated this PR to rename the field to include Ms, and throw an exception on < -1.
I added a test case for both no timeout (0), and 1000ms. I verified the test fails (doesn't terminate) without the new configuration option sent.

I don't think this is a great candidate for a random test - randomization is wonderful for perturbing data and finding edge cases in algorithms. In this case, it is just an int we compare to a clock, so randomizing doesn't seem likely to uncover any helpful edge cases. Please let me know if this is still desired.

[zhaih]

Thanks @stevenschlansker LGTM, I think this change contains no backward breaking API change so we should make it in 9.5?

[zhaih]

I merged it but seems there're test failure

org.apache.lucene.index.TestIndexFileDeleter > test suite's output saved to /home/runner/work/lucene/lucene/lucene/core/build/test-results/test/outputs/OUTPUT-org.apache.lucene.index.TestIndexFileDeleter.txt, copied below:
   >     org.apache.lucene.store.AlreadyClosedException: ReaderPool is already closed
   >         at __randomizedtesting.SeedInfo.seed([FF62209E9305A732:16FF57ACE5CC40CF]:0)
   >         at app//org.apache.lucene.index.ReaderPool.get(ReaderPool.java:400)
   >         at app//org.apache.lucene.index.IndexWriter.writeReaderPool(IndexWriter.java:3922)
   >         at app//org.apache.lucene.index.IndexWriter.getReader(IndexWriter.java:592)
   >         at app//org.apache.lucene.index.IndexWriter$4.getReader(IndexWriter.java:6479)
   >         at app//org.apache.lucene.tests.index.RandomIndexWriter.getReader(RandomIndexWriter.java:488)
   >         at app//org.apache.lucene.tests.index.RandomIndexWriter.getReader(RandomIndexWriter.java:420)
   >         at app//org.apache.lucene.index.TestIndexFileDeleter.testExcInDecRef(TestIndexFileDeleter.java:485)
   >         at java.base@17.0.4.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   >         at java.base@17.0.4.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
   >         at java.base@17.0.4.1/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >         at java.base@17.0.4.1/java.lang.reflect.Method.invoke(Method.java:568)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)
   >         at app//org.apache.lucene.tests.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:48)
   >         at app//org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
   >         at app//org.apache.lucene.tests.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
   >         at app//org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
   >         at app//org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
   >         at app//org.junit.rules.RunRules.evaluate(RunRules.java:20)
   >         at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
   >         at app//com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:390)
   >         at app//com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:843)
   >         at app//com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:490)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:955)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:840)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:891)
   >         at app//com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:902)
   >         at app//org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
   >         at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
   >         at app//org.apache.lucene.tests.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:38)
   >         at app//com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
   >         at app//com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
   >         at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
   >         at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
   >         at app//org.apache.lucene.tests.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
   >         at app//org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
   >         at app//org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
   >         at app//org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
   >         at app//org.apache.lucene.tests.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:47)
   >         at app//org.junit.rules.RunRules.evaluate(RunRules.java:20)
   >         at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
   >         at app//com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:390)
   >         at app//com.carrotsearch.randomizedtesting.ThreadLeakControl.lambda$forkTimeoutingTask$0(ThreadLeakControl.java:850)
   >         at java.base@17.0.4.1/java.lang.Thread.run(Thread.java:833)
  2> NOTE: reproduce with: gradlew test --tests TestIndexFileDeleter.testExcInDecRef -Dtests.seed=FF62209E9305A732 -Dtests.locale=is-IS -Dtests.timezone=Africa/Maseru -Dtests.asserts=true -Dtests.file.encoding=UTF-8

But seems not related to this PR, I'll create another issue if that failure is reproducible.
Thanks @stevenschlansker !

[stevenschlansker]

OK - I did run ./gradlew check so I don't think I broke anything, but please let me know if it does end up being related!

[zhaih]

I tried to reproduce the issue but couldn't. So likely a transient or extremely rare test failure, and should not be related to the PR

…

On Wed, Oct 19, 2022, 16:44 Steven Schlansker ***@***.***> wrote: OK - I did run ./gradlew check so I don't think I broke anything, but please let me know if it does end up being related! — Reply to this email directly, view it on GitHub <#11822 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEFSB7EMFIIHD5IKGTMNEKLWECBXJANCNFSM6AAAAAAQWFSERU> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>