gianm commented on code in PR #16372:
URL: https://github.com/apache/druid/pull/16372#discussion_r1592686559
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java:
##########
@@ -104,19 +109,42 @@ public GroupByResourcesReservationPool(
}
/**
- * Reserves appropriate resources, and maps it to the queryResourceId
(usually the query's resource id) in the internal map
+ * Reserves appropriate resources, and maps it to the queryResourceId
(usually the query's resource id) in the internal map.
+ * This is a blocking call, and can block up to the given query's timeout
*/
public void reserve(QueryResourceId queryResourceId, GroupByQuery
groupByQuery, boolean willMergeRunner)
{
if (queryResourceId == null) {
throw DruidException.defensive("Query resource id must be populated");
}
- pool.compute(queryResourceId, (id, existingResource) -> {
- if (existingResource != null) {
- throw DruidException.defensive("Resource with the given identifier
[%s] is already present", id);
- }
- return GroupingEngine.prepareResource(groupByQuery, mergeBufferPool,
willMergeRunner, groupByQueryConfig);
- });
+
+ // First check if the query resource id is present in the map, and if not,
populate a dummy reference. This will
+ // block other threads from populating the map with the same query id, and
is essentially same as reserving a spot in
+ // the map for the given query id. Since the actual allocation of the
resource might take longer than expected, we
+ // do it out of the critical section, once we have "reserved" the spot
+ AtomicReference<GroupByQueryResources> reference = new
AtomicReference<>(null);
+ AtomicReference<GroupByQueryResources> existingResource =
pool.putIfAbsent(queryResourceId, reference);
+
+ // Multiple attempts made to allocate the query resource for a given
resource id. Throw an exception
+ //noinspection VariableNotUsedInsideIf
+ if (existingResource != null) {
+ throw DruidException.defensive("Resource with the given identifier [%s]
is already present", queryResourceId);
+ }
+
+ GroupByQueryResources resources;
+ try {
+ // We have reserved a spot in the map. Now begin the blocking call.
+ resources = GroupingEngine.prepareResource(groupByQuery,
mergeBufferPool, willMergeRunner, groupByQueryConfig);
+ }
+ catch (Exception e) {
Review Comment:
Can you change this to `Throwable`? It isn't likely that we will actually
get a `Throwable` here that is not an `Exception`, but IMO it's good practice
for "definitely must happen" cleanup-on-failure code to catch `Throwable`
rather than `Exception`. It makes it before more like a `finally` or
try-with-resources, both of which would activate on any `Throwable`.
--
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]
