LakshSingla commented on code in PR #16372: URL: https://github.com/apache/druid/pull/16372#discussion_r1591490073
########## processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java: ########## @@ -104,19 +109,35 @@ 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); + } + + // We have reserved a spot in the map. Now begin the blocking call. + GroupByQueryResources resources = Review Comment: Great catch, it would have polluted the map. I have modified the code accordingly. -- 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: commits-unsubscr...@druid.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org