nick-boss-tech commented on PR #5223: URL: https://github.com/apache/solr/pull/5223#issuecomment-6388660203
**Review TL/DR:** P2: Cleanup can swallow an interruption. The [new catch block](https://github.com/apache/solr/blob/82eaddc7b2ebd690b99707ec26c7ffea2fd86adf/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java#L310-L316) suppresses every cleanup exception. If deletion’s collection-state wait throws InterruptedException, the interrupt flag is cleared and never restored. Restore the flag while preserving the original restore error and suppressed cleanup exception. The normal synchronous and asynchronous cleanup paths otherwise look sound, and the test covers both failures followed by a successful retry. No local tests were run. **Review Details:** 🤖 *AI text below and above* 🤖 *(reviewed by Nick Shanin)* @SpookyJumpyBeans I reviewed `82eaddc7b2eb`, including the synchronous and asynchronous shard-response handling and the collection-deletion path. The cleanup scope looks appropriate: it applies to the newly created collection before shard activation, leaves existing-collection restores unchanged, and preserves the restored collection if adding the remaining replicas fails after activation. I found one interruption-handling issue in the [[new cleanup catch block](https://github.com/apache/solr/blob/82eaddc7b2ebd690b99707ec26c7ffea2fd86adf/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java#L303-L316)](https://github.com/apache/solr/blob/82eaddc7b2ebd690b99707ec26c7ffea2fd86adf/solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java#L303-L316). If the restore fails normally and `cleanupCollection()` subsequently throws `InterruptedException`, the inner `catch (Exception cleanupException)` adds that interruption as a suppressed exception and rethrows the original restore error without restoring the thread’s interrupt flag. There is a concrete path through the current implementation: collection deletion calls `ZkStateReader.waitForState()` to wait for the collection to disappear, and that method uses `CountDownLatch.await()`. An interruption during that wait throws `InterruptedException` and clears the interrupt flag. The earlier `catch (InterruptedException)` in `RestoreCmd` does not cover exceptions thrown inside the later cleanup catch block. The distributed command runner also checks the primary exception’s type, so an interruption attached only as a suppressed exception will not restore the flag there. I suggest retaining the current exception-preservation behavior and adding the interrupt handling inside the cleanup catch: ```java } catch (Exception cleanupException) { if (cleanupException instanceof InterruptedException) { Thread.currentThread().interrupt(); } e.addSuppressed(cleanupException); } ``` That keeps the original restore failure as the primary error while preserving the interruption signal. A focused test could make the restore fail normally, make cleanup throw `InterruptedException`, and check the primary exception, suppressed exception, and interrupt status. The regression test otherwise exercises the important behavior: synchronous failure, asynchronous failure, collection absence after each, and a successful retry with active shards and the expected document count. I did not find another correctness issue in those paths. This was a static review; I did not independently run the tests or reproduce the interruption case. At review time, the upstream precommit, changelog, and test workflows were awaiting maintainer approval, so I have treated the test results described in the PR as author-reported evidence. My recommendation is to make the small interrupt-preservation change and retain the current cleanup boundaries. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
