kfaraz commented on code in PR #16420:
URL: https://github.com/apache/druid/pull/16420#discussion_r1595022786
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1522,4 +1529,10 @@ public CompactionTuningConfig
withPartitionsSpec(PartitionsSpec partitionsSpec)
);
}
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ return LookupLoadingSpec.getSpecFromContext(getContext(),
LookupLoadingSpec.NONE);
Review Comment:
I wonder if the `index_parallel` supervisor tasks launched by compaction
will ever need lookups. I should think not. So, this should probably always
return `NONE`.
cc: @cryptoe
##########
server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java:
##########
@@ -80,6 +88,34 @@ public ImmutableSet<String> getLookupsToLoad()
return lookupsToLoad;
}
+ public static LookupLoadingSpec getSpecFromContext(Map<String, Object>
context, LookupLoadingSpec defaultSpec)
Review Comment:
```suggestion
public static LookupLoadingSpec createFromContext(Map<String, Object>
context, LookupLoadingSpec defaultSpec)
```
##########
server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java:
##########
@@ -39,6 +43,10 @@
*/
public class LookupLoadingSpec
{
+
+ public static final String CTX_LOOKUP_LOADING_MODE = "lookupLoadingMode";
+ public static final String CTX_LOOKUPS_TO_LOAD = "lookupsToLoad";
Review Comment:
I think these constants should live in `Tasks` class instead.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -249,6 +250,12 @@ public CompactionTask(
this.segmentProvider = new SegmentProvider(dataSource,
this.ioConfig.getInputSpec());
this.partitionConfigurationManager = new
PartitionConfigurationManager(this.tuningConfig);
this.segmentCacheManagerFactory = segmentCacheManagerFactory;
+
+ // Unless context has been overridden to load lookups differently, we want
to load no lookups by default in any task spawned
+ // up by the CompactionTask. We achieve this by populating this info in
the context which is passed to the spawned tasks.
Review Comment:
```suggestion
// By default, do not load any lookups in sub-tasks launched by
compaction task
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -249,6 +250,12 @@ public CompactionTask(
this.segmentProvider = new SegmentProvider(dataSource,
this.ioConfig.getInputSpec());
this.partitionConfigurationManager = new
PartitionConfigurationManager(this.tuningConfig);
this.segmentCacheManagerFactory = segmentCacheManagerFactory;
+
+ // Unless context has been overridden to load lookups differently, we want
to load no lookups by default in any task spawned
+ // up by the CompactionTask. We achieve this by populating this info in
the context which is passed to the spawned tasks.
+ if (context == null ||
!context.containsKey(LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE)) {
+ addToContext(LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE,
LookupLoadingSpec.Mode.NONE.toString());
+ }
Review Comment:
Use method `addToContextIfAbsent()`.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -357,7 +355,7 @@ public void addLookupToLoad(String lookupName)
}
/**
- * Returns the lookup to load for a given task.
+ * Returns the lookup loading spec for a given task.
Review Comment:
```suggestion
* Lookup loading spec used if this context corresponds to an MSQ task.
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -334,9 +334,13 @@ static TaskInfo<TaskIdentifier, TaskStatus>
toTaskIdentifierInfo(TaskInfo<Task,
);
}
+ /**
+ * Fetch lookup loading spec based on the task context, defaulting to
loading all lookups if context does not have overridden configuration.
+ * @return lookup loading spec
Review Comment:
```suggestion
* Specifies the list of lookups to load for this task. Tasks load ALL
lookups by default.
* This behaviour can be overridden by passing parameters {@link
Tasks#KEY_LOOKUP_LOADING_MODE}
* and {@link Tasks#KEY_LOOKUPS_TO_LOAD} in the task context.
```
--
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]
