You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "psalagnac (via GitHub)" <gi...@apache.org> on 2023/07/06 22:13:48 UTC
[GitHub] [solr] psalagnac opened a new pull request, #1761: Admin queue
psalagnac opened a new pull request, #1761:
URL: https://github.com/apache/solr/pull/1761
A draft PR with a queue for some expensive async tasks.
--
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] psalagnac commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1261617512
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -403,60 +411,109 @@ public interface CoreAdminOp {
public static class CoreAdminAsyncTracker {
private static final int MAX_TRACKED_REQUESTS = 100;
+ public static final String PENDING = "pending";
public static final String RUNNING = "running";
public static final String COMPLETED = "completed";
public static final String FAILED = "failed";
public final Map<String, Map<String, TaskObject>> requestStatusMap;
+ static final int MAX_CONCURRENT_EXPENSIVE_TASKS =
+ Integer.getInteger("solr.admin.maxConcurrentExpensiveTasks", 5);
+ private volatile boolean shutdown;
+
+ // Both expensiveRunningTasks and expensiveTaskQueue are guarded by this
+ private int expensiveRunningTasks;
+ private final Queue<TaskObject> expensiveTaskQueue;
+
private ExecutorService parallelExecutor =
ExecutorUtil.newMDCAwareFixedThreadPool(
50, new SolrNamedThreadFactory("parallelCoreAdminAPIBaseExecutor"));
public CoreAdminAsyncTracker() {
HashMap<String, Map<String, TaskObject>> map = new HashMap<>(3, 1.0f);
+ map.put(PENDING, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(RUNNING, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(COMPLETED, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(FAILED, Collections.synchronizedMap(new LinkedHashMap<>()));
requestStatusMap = Collections.unmodifiableMap(map);
+
+ expensiveTaskQueue = new LinkedList<>();
}
public void shutdown() {
+
+ shutdown = true;
+
+ // Before shutting down the thread pool, we have to execute all the expensive tasks that
+ // are already in the queue
+ synchronized (this) {
+ if (!expensiveTaskQueue.isEmpty()) {
Review Comment:
Good point. We can have a _spurious_ wakeup here. 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
[GitHub] [solr] dsmiley closed pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley closed pull request #1761: SOLR-16879: add queue for expensive admin operations
URL: https://github.com/apache/solr/pull/1761
--
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] psalagnac commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1289053033
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -506,30 +507,23 @@ private static NamedList<Object> waitForCoreAdminAsyncCallToComplete(
}
String r = (String) srsp.getSolrResponse().getResponse().get("STATUS");
- if (r.equals("running")) {
Review Comment:
I may miss something. Do you suggest to introduce a new enum for this? I don't think there is one for request statuses.
Here `PENDING` is just a constant string.
--
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] dsmiley commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1286173971
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -506,30 +507,23 @@ private static NamedList<Object> waitForCoreAdminAsyncCallToComplete(
}
String r = (String) srsp.getSolrResponse().getResponse().get("STATUS");
- if (r.equals("running")) {
Review Comment:
Why don't we change this to a `switch` statement? Even IntelliJ is recommending this :-) It seems an underlying bug here would have been averted had a switch statement been used as it sort of forces you to consider all the enum values.
--
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] dsmiley commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1671963201
I like Alex's idea of a second thread pool / executor; I think it would mean we would not need the explicit expensiveTaskQueue that is fiddly to work with and could rely on the more familiar Executor (that has an impl using a queue but it's not our code to worry about). But I don't have a strong opinion.
> This is at the cost of having many dormant threads most of the time.
`org.apache.solr.common.util.ExecutorUtil#newMDCAwareCachedThreadPool(int, java.util.concurrent.ThreadFactory)` will not keep unused threads around long; only a minute.
--
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] dsmiley commented on a diff in pull request #1761: Admin queue
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1255883240
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -403,60 +410,106 @@ public interface CoreAdminOp {
public static class CoreAdminAsyncTracker {
private static final int MAX_TRACKED_REQUESTS = 100;
+ public static final String PENDING = "pending";
public static final String RUNNING = "running";
public static final String COMPLETED = "completed";
public static final String FAILED = "failed";
public final Map<String, Map<String, TaskObject>> requestStatusMap;
+ static final int MAX_CONCURRENT_EXPENSIVE_TASKS =
+ Integer.getInteger("solr.admin.maxConcurrentExpensiveTasks", 5);
+ private volatile boolean shutdown;
+ private int expensiveRunningTasks;
+ private final ConcurrentLinkedQueue<TaskObject> expensiveTaskQueue;
Review Comment:
Not actually used concurrently though
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -218,13 +217,16 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
if (taskId == null) {
callInfo.call();
} else {
- coreAdminAsyncTracker.submitAsyncTask(
- taskObject,
- action,
+ Callable<SolrQueryResponse> task =
() -> {
callInfo.call();
return callInfo.rsp;
- });
+ };
+
+ final CoreAdminAsyncTracker.TaskObject taskObject =
Review Comment:
Just want to mention that Java 11 "var" keyword would be useful here where there is a very long type, and we know the type clearly here anyway. Or just inline this.
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -403,60 +410,106 @@ public interface CoreAdminOp {
public static class CoreAdminAsyncTracker {
private static final int MAX_TRACKED_REQUESTS = 100;
+ public static final String PENDING = "pending";
public static final String RUNNING = "running";
public static final String COMPLETED = "completed";
public static final String FAILED = "failed";
public final Map<String, Map<String, TaskObject>> requestStatusMap;
+ static final int MAX_CONCURRENT_EXPENSIVE_TASKS =
+ Integer.getInteger("solr.admin.maxConcurrentExpensiveTasks", 5);
+ private volatile boolean shutdown;
+ private int expensiveRunningTasks;
+ private final ConcurrentLinkedQueue<TaskObject> expensiveTaskQueue;
+
private ExecutorService parallelExecutor =
ExecutorUtil.newMDCAwareFixedThreadPool(
50, new SolrNamedThreadFactory("parallelCoreAdminAPIBaseExecutor"));
public CoreAdminAsyncTracker() {
HashMap<String, Map<String, TaskObject>> map = new HashMap<>(3, 1.0f);
+ map.put(PENDING, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(RUNNING, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(COMPLETED, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(FAILED, Collections.synchronizedMap(new LinkedHashMap<>()));
requestStatusMap = Collections.unmodifiableMap(map);
+
+ expensiveTaskQueue = new ConcurrentLinkedQueue<>();
}
public void shutdown() {
+
+ shutdown = true;
+
+ // Before shutting down the thread pool, we have to execute all the expensive tasks that
+ // are already in the queue
+ synchronized (this) {
+ if (!expensiveTaskQueue.isEmpty()) {
+ try {
+ wait(60000L);
Review Comment:
comment: notifyAll is in finishTask()
--
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] dsmiley commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1663243263
Once solr/CHANGES.txt is updated, I think it's ready to merge
--
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] bruno-roustant commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1258392079
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -403,60 +411,109 @@ public interface CoreAdminOp {
public static class CoreAdminAsyncTracker {
private static final int MAX_TRACKED_REQUESTS = 100;
+ public static final String PENDING = "pending";
public static final String RUNNING = "running";
public static final String COMPLETED = "completed";
public static final String FAILED = "failed";
public final Map<String, Map<String, TaskObject>> requestStatusMap;
+ static final int MAX_CONCURRENT_EXPENSIVE_TASKS =
+ Integer.getInteger("solr.admin.maxConcurrentExpensiveTasks", 5);
+ private volatile boolean shutdown;
+
+ // Both expensiveRunningTasks and expensiveTaskQueue are guarded by this
+ private int expensiveRunningTasks;
+ private final Queue<TaskObject> expensiveTaskQueue;
+
private ExecutorService parallelExecutor =
ExecutorUtil.newMDCAwareFixedThreadPool(
50, new SolrNamedThreadFactory("parallelCoreAdminAPIBaseExecutor"));
public CoreAdminAsyncTracker() {
HashMap<String, Map<String, TaskObject>> map = new HashMap<>(3, 1.0f);
+ map.put(PENDING, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(RUNNING, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(COMPLETED, Collections.synchronizedMap(new LinkedHashMap<>()));
map.put(FAILED, Collections.synchronizedMap(new LinkedHashMap<>()));
requestStatusMap = Collections.unmodifiableMap(map);
+
+ expensiveTaskQueue = new LinkedList<>();
}
public void shutdown() {
+
+ shutdown = true;
+
+ // Before shutting down the thread pool, we have to execute all the expensive tasks that
+ // are already in the queue
+ synchronized (this) {
+ if (!expensiveTaskQueue.isEmpty()) {
Review Comment:
Should it be a while?
--
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] stillalex commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1670292382
read through this, I'm not really familiar with any of this code but the change looks good to me.
minor question. why not add another executor (fixed number of threads similar to parallelExecutor) for these expensive operations? all the code that ensures the max number of running threads would no longer be necessary, the finishTask method would also be a bit simpler.
another one for my personal understanding: when the status of a task is reported as `RUNNING` it could either be queued or running. this means 'sent to executor', but there is no guarantee this will run immediately. introducing another status as `PENDING` might cause some confusion to what is actually running at a given moment.
--
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] sonatype-lift[bot] commented on a diff in pull request #1761: Admin queue
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1255006633
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -482,7 +535,8 @@ private void removeTask(String map, String taskId) {
}
private void ensureTaskIdNotInUse(String taskId) throws SolrException {
- if (getRequestStatusMap(RUNNING).containsKey(taskId)
+ if (getRequestStatusMap(PENDING).containsKey(taskId)
Review Comment:
<picture><img alt="7% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/7/display.svg"></picture>
<b>*NULLPTR_DEREFERENCE:</b>* `Map CoreAdminHandler$CoreAdminAsyncTracker.getRequestStatusMap(String)` could be null (from the call to `CoreAdminHandler$CoreAdminAsyncTracker.getRequestStatusMap(...)` on line 538) and is dereferenced.
---
<details><summary>ℹ️ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
--
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] psalagnac commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1288526660
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -506,30 +507,23 @@ private static NamedList<Object> waitForCoreAdminAsyncCallToComplete(
}
String r = (String) srsp.getSolrResponse().getResponse().get("STATUS");
- if (r.equals("running")) {
Review Comment:
We don't check an enum, but string values.
I often feel a `switch` statement on strings is error prone, and doesn't protect from regressions.
What do you think?
--
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] psalagnac commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1671353210
> Any thoughts on how this bug slipped by our notice?
I was unable to write a unit that actually do an expensive operation (backup or restore). The low-level queue mechanism is well tested, but I'm not sure how to write an end-to-end test with expensive operations at scale.
> read through this, I'm not really familiar with any of this code but the change looks good to me. minor question. why not add another executor (fixed number of threads similar to parallelExecutor) for these expensive operations? all the code that ensures the max number of running threads would no longer be necessary, the finishTask method would also be a bit simpler.
Yes, that would be another option.
This is at the cost of having many dormant threads most of the time. Also, manually handling a queue for expensive tasks allow eventual future improvements.
I don't have any strong opinion.
>
> another one for my personal understanding: when the status of a task is reported as `RUNNING` it could either be queued or running. this means 'sent to executor', but there is no guarantee this will run immediately. introducing another status as `PENDING` might cause some confusion to what is actually running at a given moment.
You're correct, RUNNING status means 'sent to executor'.
For most of the tasks, I don't expect the time spend by the task in the executor queue to be significant (otherwise, we would have to mark something else as expensive if it keeps the executor busy for a while).
I added the `PENDING` status to make this difference. Expensive tasks can be in the queue for seconds/minutes for big collections, so I think there is some value for the caller to know that at some point.
(note that the caller is the overseer only. The top level task with user specified asyncId does not expose this _pending_ status)
--
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] psalagnac commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1664769445
While testing this internally, I found a minor issue (will push a fix soon). The flag to set some operation as expensive is not correctly passed.
I'd rather we wait more before merging.
--
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] dsmiley commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1701560186
Closed in favor of #1864
--
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] psalagnac commented on pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on PR #1761:
URL: https://github.com/apache/solr/pull/1761#issuecomment-1667510102
Hi @dsmiley,
I think this change is now ready to be merged.
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1289046189
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -506,30 +507,23 @@ private static NamedList<Object> waitForCoreAdminAsyncCallToComplete(
}
String r = (String) srsp.getSolrResponse().getResponse().get("STATUS");
- if (r.equals("running")) {
Review Comment:
I'm suggesting the switch cases should be on the enum instances, not string literals.
--
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] dsmiley commented on a diff in pull request #1761: SOLR-16879: add queue for expensive admin operations
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1761:
URL: https://github.com/apache/solr/pull/1761#discussion_r1289128149
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -506,30 +507,23 @@ private static NamedList<Object> waitForCoreAdminAsyncCallToComplete(
}
String r = (String) srsp.getSolrResponse().getResponse().get("STATUS");
- if (r.equals("running")) {
Review Comment:
`org.apache.solr.client.solrj.response.RequestStatusState` but it looks like I confused collection async status with core async status. Too bad they don't use the same enum!
--
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