dsmiley commented on code in PR #2304:
URL: https://github.com/apache/solr/pull/2304#discussion_r1509871223
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -448,25 +458,51 @@ public static class CoreAdminAsyncTracker {
new
SolrNamedThreadFactory("parallelCoreAdminAPIExpensiveExecutor"));
public CoreAdminAsyncTracker() {
- HashMap<String, Map<String, TaskObject>> map = new HashMap<>(3, 1.0f);
- 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);
+ this(
+ Ticker.systemTicker(),
+ TimeUnit.MINUTES.toNanos(
+ Long.getLong("solr.admin.requests.running.timeout.minutes",
60L)),
Review Comment:
Use EnvUtils -- a new thing
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -546,6 +559,8 @@ public static class TaskObject {
final Callable<SolrQueryResponse> task;
public String rspInfo;
public Object operationRspInfo;
+ private volatile String status;
+ private volatile boolean polled;
Review Comment:
Could use javadoc. From observing you code, this is set to true once the
status has been requested AND it is no longer RUNNING. Maybe a different name
like `polledAfterCompletion` would be clearer.
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -427,11 +430,18 @@ default boolean isExpensive() {
}
public static class CoreAdminAsyncTracker {
- private static final int MAX_TRACKED_REQUESTS = 100;
+ /**
+ * Max number of requests we track in the Caffeine cache. This limit is
super high on
+ * purpose, we're not supposed to hit it. This is just a protection to
grow in memory too
+ * much when receiving an abusive number of admin requests.
+ */
+ private static final int MAX_TRACKED_REQUESTS = 10_000;
+
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;
+
+ private final Cache<String, TaskObject> requestStatusCache;
Review Comment:
```suggestion
private final Cache<String, TaskObject> requestStatusCache; // key by ID
```
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -497,42 +533,19 @@ public void submitAsyncTask(TaskObject taskObject) throws
SolrException {
}
}
- /** Helper method to add a task to a tracking type. */
- private void addTask(String type, TaskObject o, boolean limit) {
- synchronized (getRequestStatusMap(type)) {
- if (limit && getRequestStatusMap(type).size() == MAX_TRACKED_REQUESTS)
{
- String key =
getRequestStatusMap(type).entrySet().iterator().next().getKey();
- getRequestStatusMap(type).remove(key);
- }
- addTask(type, o);
- }
- }
-
- private void addTask(String type, TaskObject o) {
- synchronized (getRequestStatusMap(type)) {
- getRequestStatusMap(type).put(o.taskId, o);
- }
- }
-
- /** Helper method to remove a task from a tracking map. */
- private void removeTask(String map, String taskId) {
- synchronized (getRequestStatusMap(map)) {
- getRequestStatusMap(map).remove(taskId);
- }
- }
-
- private void ensureTaskIdNotInUse(String taskId) throws SolrException {
- if (getRequestStatusMap(RUNNING).containsKey(taskId)
- || getRequestStatusMap(COMPLETED).containsKey(taskId)
- || getRequestStatusMap(FAILED).containsKey(taskId)) {
+ private void addTask(TaskObject taskObject) {
+ // Ensure task ID is not already in use
+ if (requestStatusCache.asMap().containsKey(taskObject.taskId)) {
Review Comment:
I think there's a race where this check is false for 2 threads and both get
entered into the cache via put. If you instead call `get` (with lambda to
supply the new value) , you can check if the returned value is the new
taskObject or is an existing one.
##########
solr/core/src/test/org/apache/solr/handler/admin/CoreAdminHandlerTest.java:
##########
@@ -528,4 +534,39 @@ public void testNonexistentCoreReload() throws Exception {
e.getMessage());
admin.close();
}
+
+ @Test
+ public void testTrackedRequestExpiration() throws Exception {
Review Comment:
wonderful test
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeCommandStatus.java:
##########
@@ -51,25 +51,24 @@ public GetNodeCommandStatus(
public GetNodeCommandStatusResponse getCommandStatus(String requestId) {
ensureRequiredParameterProvided(CoreAdminParams.REQUESTID, requestId);
var requestStatusResponse = new GetNodeCommandStatusResponse();
- if
(coreAdminAsyncTracker.getRequestStatusMap(RUNNING).containsKey(requestId)) {
- requestStatusResponse.status = RUNNING;
- } else if
(coreAdminAsyncTracker.getRequestStatusMap(COMPLETED).containsKey(requestId)) {
- requestStatusResponse.status = COMPLETED;
- requestStatusResponse.response =
-
coreAdminAsyncTracker.getRequestStatusMap(COMPLETED).get(requestId).getRspObject();
- requestStatusResponse.response =
- coreAdminAsyncTracker
- .getRequestStatusMap(COMPLETED)
- .get(requestId)
- .getOperationRspObject();
- } else if
(coreAdminAsyncTracker.getRequestStatusMap(FAILED).containsKey(requestId)) {
- requestStatusResponse.status = FAILED;
- requestStatusResponse.response =
-
coreAdminAsyncTracker.getRequestStatusMap(FAILED).get(requestId).getRspObject();
- } else {
+
+ TaskObject taskObject =
coreAdminAsyncTracker.getAsyncRequestForStatus(requestId);
+ String status = taskObject != null ? taskObject.getStatus() : null;
+
+ if (status == null) {
requestStatusResponse.status = "notfound";
requestStatusResponse.msg = "No task found in running, completed or
failed tasks";
+ } else {
+ requestStatusResponse.status = status;
+
+ if (taskObject.getStatus().equals(COMPLETED)) {
Review Comment:
can reference `status`
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeCommandStatus.java:
##########
@@ -51,25 +51,24 @@ public GetNodeCommandStatus(
public GetNodeCommandStatusResponse getCommandStatus(String requestId) {
ensureRequiredParameterProvided(CoreAdminParams.REQUESTID, requestId);
var requestStatusResponse = new GetNodeCommandStatusResponse();
- if
(coreAdminAsyncTracker.getRequestStatusMap(RUNNING).containsKey(requestId)) {
- requestStatusResponse.status = RUNNING;
- } else if
(coreAdminAsyncTracker.getRequestStatusMap(COMPLETED).containsKey(requestId)) {
- requestStatusResponse.status = COMPLETED;
- requestStatusResponse.response =
-
coreAdminAsyncTracker.getRequestStatusMap(COMPLETED).get(requestId).getRspObject();
- requestStatusResponse.response =
- coreAdminAsyncTracker
- .getRequestStatusMap(COMPLETED)
- .get(requestId)
- .getOperationRspObject();
- } else if
(coreAdminAsyncTracker.getRequestStatusMap(FAILED).containsKey(requestId)) {
- requestStatusResponse.status = FAILED;
- requestStatusResponse.response =
-
coreAdminAsyncTracker.getRequestStatusMap(FAILED).get(requestId).getRspObject();
- } else {
+
+ TaskObject taskObject =
coreAdminAsyncTracker.getAsyncRequestForStatus(requestId);
+ String status = taskObject != null ? taskObject.getStatus() : null;
+
+ if (status == null) {
requestStatusResponse.status = "notfound";
requestStatusResponse.msg = "No task found in running, completed or
failed tasks";
+ } else {
+ requestStatusResponse.status = status;
+
+ if (taskObject.getStatus().equals(COMPLETED)) {
+ requestStatusResponse.response = taskObject.getRspObject();
+ requestStatusResponse.response = taskObject.getOperationRspObject();
Review Comment:
I'm confused; you are setting the response twice?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
