LakshSingla commented on code in PR #16372:
URL: https://github.com/apache/druid/pull/16372#discussion_r1588760113
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java:
##########
@@ -104,18 +104,23 @@ 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 upto 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");
}
+ GroupByQueryResources resources =
+ GroupingEngine.prepareResource(groupByQuery, mergeBufferPool,
willMergeRunner, groupByQueryConfig);
+
pool.compute(queryResourceId, (id, existingResource) -> {
if (existingResource != null) {
+ resources.close();
Review Comment:
The change that you suggested seems much cleaner.
While testing if the method throws, I found that the method can prematurely
block instead of throwing with the duplicate query id exception. This is
because we are allocating the resources first, and then entering it into the
map. While we never expect duplicate query resource IDs to be present, we still
want the defensive check to be thrown as soon as possible, instead of being
blocked for the merge buffers to be free (note: it's not a deadlock, but an
inconvenience).
I have made some changes to alleviate this issue.
--
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]
