jihoonson commented on a change in pull request #10082:
URL: https://github.com/apache/druid/pull/10082#discussion_r448136240



##########
File path: 
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java
##########
@@ -329,15 +329,18 @@ private DataSource inlineIfNecessary(
         }
       } else if (canRunQueryUsingLocalWalker(subQuery) || 
canRunQueryUsingClusterWalker(subQuery)) {
         // Subquery needs to be inlined. Assign it a subquery id and run it.
-        final Query subQueryWithId = 
subQuery.withSubQueryId(UUID.randomUUID().toString());
+        final Query subQueryWithId = subQuery.withDefaultSubQueryId();
 
         final Sequence<?> queryResults;
 
         if (dryRun) {
           queryResults = Sequences.empty();
         } else {
           final QueryRunner subqueryRunner = subQueryWithId.getRunner(this);
-          queryResults = subqueryRunner.run(QueryPlus.wrap(subQueryWithId));
+          queryResults = subqueryRunner.run(
+              QueryPlus.wrap(subQueryWithId),
+              DirectDruidClient.makeResponseContextForQuery()

Review comment:
       > Now that I think about it, though, making a new context here will mean 
we aren't going to properly return context from subqueries up to the original 
caller. This includes not reporting missing segments in the case where 
`RetryQueryRunnerConfig.isReturnPartialResults = true`.
   
   Thanks. Looking at the keys in responseContext, it makes sense to share it 
between subqueries (at least for some of them such as `cpuConsumed`) and seems 
doable. But I would like to do it in a separate PR since this PR is already 
big. I will document it as a known issue.
   
   BTW, the responseContext seems to be used for diverse purposes now. Some of 
them are:
   
   - To inform something important to users after query is done, such as 
`uncoveredIntervals`. 
   - To store some intermediate state in the broker during query processing, 
such as `remainingResponsesFromQueryServers`.
   - To gather some metrics from query servers such as `cpuConsumed`.
   - A part of the cache validation mechanism using `ETag`.
   
   I think it would probably be better split those into the first one and 
others at some point.




----------------------------------------------------------------
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.

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

Reply via email to