dsmiley commented on code in PR #2304:
URL: https://github.com/apache/solr/pull/2304#discussion_r1638975112
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -497,42 +534,23 @@ 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(TaskObject taskObject) {
+ // Ensure task ID is not already in use
+ TaskObject taskInCache = requestStatusCache.get(taskObject.taskId, n ->
taskObject);
- 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)) {
+ // If we get a different task instance, it means one was already in the
cache with the
+ // same name. Just reject the new one.
+ if (taskInCache != taskObject) {
throw new SolrException(
ErrorCode.BAD_REQUEST, "Duplicate request with the same requestid
found.");
}
+
+ taskObject.status = RUNNING;
+ requestStatusCache.put(taskObject.taskId, taskObject);
Review Comment:
The call to `get` with the lambda above (L539) will insert it atomically, so
you shouldn't try to put it *again* here, thus maybe having some races.
Generally try to avoid double operations with a cache -- use one (atomic)
operation. Also, you should set the status to RUNNING before then so that a
race will be sure to see RUNNING.
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -427,11 +431,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;
Review Comment:
Use EnvUtils.getPropertyAsLong here too; perhaps
"solr.admin.requests.running.max"
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -448,25 +459,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(
+
EnvUtils.getEnvAsLong("solr.admin.requests.running.timeout.minutes", 60L)),
Review Comment:
getPropertyAsLong as the string here is clearly not env-looking (nor should
it be)
--
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
