You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "patsonluk (via GitHub)" <gi...@apache.org> on 2023/03/14 22:30:10 UTC
[GitHub] [solr] patsonluk opened a new pull request, #1460: SOLR-16701: Race condition on PRS enabled collection deletion
patsonluk opened a new pull request, #1460:
URL: https://github.com/apache/solr/pull/1460
https://issues.apache.org/jira/browse/SOLR-16701
# Description
This fixes a race condition on PRS enabled collection deletion, which triggers the exception:
```
org.apache.solr.common.SolrException: Error fetching per-replica states
at __randomizedtesting.SeedInfo.seed([C2BFFBF8FE49C1E1:F1C8D9E308D2745]:0)
at app//org.apache.solr.common.cloud.PerReplicaStatesFetcher.fetch(PerReplicaStatesFetcher.java:49)
at app//org.apache.solr.common.cloud.PerReplicaStatesFetcher$LazyPrsSupplier.lambda$new$0(PerReplicaStatesFetcher.java:62)
at app//org.apache.solr.common.cloud.DocCollection$PrsSupplier.get(DocCollection.java:515)
at app//org.apache.solr.common.cloud.Replica.isLeader(Replica.java:314)
at app//org.apache.solr.common.cloud.Slice.findLeader(Slice.java:242)
at app//org.apache.solr.common.cloud.Slice.setPrsSupplier(Slice.java:56)
at app//org.apache.solr.common.cloud.DocCollection.<init>(DocCollection.java:123)
at app//org.apache.solr.common.cloud.ClusterState.collectionFromObjects(ClusterState.java:305)
at app//org.apache.solr.common.cloud.ClusterState.createFromCollectionMap(ClusterState.java:254)
at app//org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider.createFromJsonSupportingLegacyConfigName(ZkClientClusterStateProvider.java:117)
at app//org.apache.solr.common.cloud.ZkStateReader.fetchCollectionState(ZkStateReader.java:1695)
```
This could be triggered by:
1. `fetchCollectionState` is called, and the state.json is fetched
2. But before the `fetchCollectionState` fetches the PRS entries, the collection state.json/PRS are deleted by someone else
3. `fetchCollectionState` would throw below exception when it reaches the PRS fetching logic as the Zk node state.json is no longer around
# Solution
Create a specific exception `PrsZkNodeNotFoundException` (that extends `SolrException`) when the PRS entries cannot be fetched. Then in `ZkStateReader#fetchCollectionState`, catch this exception as well (along with the existing `KeeperException.NoNodeException`), and use the same handling to fetch the state again.
# Tests
Added `ZkStateReaderTest#testDeletePrsCollection` which reproduce such race condition, and verify that:
1. The `ZkStateReader#fetchCollectionState` should not throw exception, instead, it should eventually return `null` which indicates the collection is deleted
2. The `PrsZkNodeNotFoundException` was indeed triggered
Please take note that the test case was built on the `Breakpoint` introduced by another PR https://github.com/apache/solr/pull/1457
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney merged pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney merged PR #1460:
URL: https://github.com/apache/solr/pull/1460
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1323505098
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1627,6 +1627,20 @@ private DocCollection fetchCollectionState(String coll, Watcher watcher)
}
}
return null;
+ } catch (PerReplicaStatesOps.PrsZkNodeNotFoundException e) {
+ CommonTestInjection.injectBreakpoint(ZkStateReader.class.getName() + "/exercised", e);
Review Comment:
should be invoked as an `assert` statement
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,68 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ * @param args optional arguments list to be passed to the Breakpoint
+ */
+ public static boolean injectBreakpoint(String key, Object... args) {
Review Comment:
The way this is used currently, we're passing a single arg (the exception). For a utility class like this I guess I'd slightly prefer to have the signature be a _single_ arg, esp. since the way this will _ever_ be used, afaict, would involve close coordination between caller and breakpoint-setter. Basically: if an array needs to be passed, the calling code can just construct an array. I don't think varargs gets us anything much here. That said, I don't feel too strongly about it either way.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1330700685
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,68 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ * @param args optional arguments list to be passed to the Breakpoint
+ */
+ public static boolean injectBreakpoint(String key, Object... args) {
Review Comment:
I do personally like the flexibilty of the varargs, especially for most cases when there's no need for any args, then the invocation can just be `injectBreakpoint("mykey")` instead of `injectBreakpoint("mykey", null)`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1333392182
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,68 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ * @param args optional arguments list to be passed to the Breakpoint
+ */
+ public static boolean injectBreakpoint(String key, Object... args) {
Review Comment:
That makes sense; I'm fine with that.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1334779433
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -129,15 +111,61 @@ public static boolean injectBreakpoint(String key, Object... args) {
log.info("Breakpoint with key {} is triggered", key);
breakpoint.executeAndResume(args);
log.info("Breakpoint with key {} was executed and normal code execution resumes", key);
+ } else {
+ log.info(
+ "Breakpoint with key {} is triggered but there's no implementation set. Skipping...",
+ key);
Review Comment:
yes let's change that to debug, i was a bit concerned if someone accidentally used different key names meant for the same Breakpoint for `injectBreakpoint` and `setImplementation`, so with this log message they can see whether the breakpoint is triggered.
Though I guess this could be really noisy as you pointed out, and I assume the dev can search whether there's "Breakpoint with key ... is triggered" message instead and absence of that means either the execution path does not reach the breakpoint or the breakpoint has no implementation set
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1334709570
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -129,15 +111,61 @@ public static boolean injectBreakpoint(String key, Object... args) {
log.info("Breakpoint with key {} is triggered", key);
breakpoint.executeAndResume(args);
log.info("Breakpoint with key {} was executed and normal code execution resumes", key);
+ } else {
+ log.info(
+ "Breakpoint with key {} is triggered but there's no implementation set. Skipping...",
+ key);
Review Comment:
I don't think we should log here (and fwiw there is no corresponding logging for absent `injectDelay()`. The issue is that if you were to enable assertions on a running system (not in a test context), you might get a ton of spam from this. If you want to leave it, maybe set log level from `log.info()` to `log.debug()`?
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -129,15 +111,61 @@ public static boolean injectBreakpoint(String key, Object... args) {
log.info("Breakpoint with key {} is triggered", key);
breakpoint.executeAndResume(args);
log.info("Breakpoint with key {} was executed and normal code execution resumes", key);
+ } else {
+ log.info(
+ "Breakpoint with key {} is triggered but there's no implementation set. Skipping...",
+ key);
}
return true;
}
public interface Breakpoint {
/**
* Code execution should break at where the breakpoint was injected, then it would execute this
- * method and resumes the execution afterwards.
+ * method and resumes the execution afterward.
*/
void executeAndResume(Object... args);
}
+
+ /**
+ * Breakpoints should be set via this {@link BreakpointSetter} within the test case and close
+ * should be invoked as cleanup. Since this is closeable, it should usually be used in the
+ * try-with-resource syntax, such as:
+ *
+ * <pre>{@code
+ * try (BreakpointSetter breakpointSetter = new BreakpointSetter() {
+ * //... test code here that calls breakpointSetter.setImplementation(...)
+ * }
+ * }</pre>
+ */
+ public static class BreakpointSetter implements Closeable {
+ private Set<String> keys = new HashSet<>();
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If a breakpoint implementation is set by this method, then code execution would break at
+ * the code execution point marked by CommonTestInjection#injectBreakpoint with matching key,
+ * executes the provided implementation in the {@link Breakpoint}, then resumes the normal code
+ * execution.
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param implementation The Breakpoint implementation
+ */
+ public void setImplementation(String key, Breakpoint implementation) {
+ if (breakpoints.containsKey(key)) {
+ throw new IllegalArgumentException(
+ "Cannot refine Breakpoint implementation with key " + key);
Review Comment:
nit: "modify" or "replace" rather than "refine"? We'd be replacing, not refining.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1334779855
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -129,15 +111,61 @@ public static boolean injectBreakpoint(String key, Object... args) {
log.info("Breakpoint with key {} is triggered", key);
breakpoint.executeAndResume(args);
log.info("Breakpoint with key {} was executed and normal code execution resumes", key);
+ } else {
+ log.info(
+ "Breakpoint with key {} is triggered but there's no implementation set. Skipping...",
+ key);
}
return true;
}
public interface Breakpoint {
/**
* Code execution should break at where the breakpoint was injected, then it would execute this
- * method and resumes the execution afterwards.
+ * method and resumes the execution afterward.
*/
void executeAndResume(Object... args);
}
+
+ /**
+ * Breakpoints should be set via this {@link BreakpointSetter} within the test case and close
+ * should be invoked as cleanup. Since this is closeable, it should usually be used in the
+ * try-with-resource syntax, such as:
+ *
+ * <pre>{@code
+ * try (BreakpointSetter breakpointSetter = new BreakpointSetter() {
+ * //... test code here that calls breakpointSetter.setImplementation(...)
+ * }
+ * }</pre>
+ */
+ public static class BreakpointSetter implements Closeable {
+ private Set<String> keys = new HashSet<>();
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If a breakpoint implementation is set by this method, then code execution would break at
+ * the code execution point marked by CommonTestInjection#injectBreakpoint with matching key,
+ * executes the provided implementation in the {@link Breakpoint}, then resumes the normal code
+ * execution.
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param implementation The Breakpoint implementation
+ */
+ public void setImplementation(String key, Breakpoint implementation) {
+ if (breakpoints.containsKey(key)) {
+ throw new IllegalArgumentException(
+ "Cannot refine Breakpoint implementation with key " + key);
Review Comment:
ahh was a typo, supposed to be "redefine"
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1330700685
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,68 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ * @param args optional arguments list to be passed to the Breakpoint
+ */
+ public static boolean injectBreakpoint(String key, Object... args) {
Review Comment:
I do personally like the flexibility of the varargs, especially for most cases when there's no need for any args, then the invocation can just be `injectBreakpoint("mykey")` instead of `injectBreakpoint("mykey", null)`
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,68 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String, Object...)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in injectBreakpoint
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ * @param args optional arguments list to be passed to the Breakpoint
+ */
+ public static boolean injectBreakpoint(String key, Object... args) {
Review Comment:
I do personally like the flexibility of the varargs, especially for most cases when there's no need for any args, then the invocation can just be `injectBreakpoint("mykey")` instead of `injectBreakpoint("mykey", null)`
WDYT? 😊
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1460: SOLR-16701: Race condition on PRS enabled collection deletion
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1460:
URL: https://github.com/apache/solr/pull/1460#discussion_r1330699417
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1627,6 +1627,20 @@ private DocCollection fetchCollectionState(String coll, Watcher watcher)
}
}
return null;
+ } catch (PerReplicaStatesOps.PrsZkNodeNotFoundException e) {
+ CommonTestInjection.injectBreakpoint(ZkStateReader.class.getName() + "/exercised", e);
Review Comment:
good catch thanks!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org