cryptoe commented on code in PR #16420:
URL: https://github.com/apache/druid/pull/16420#discussion_r1597782074
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -249,6 +250,14 @@ public CompactionTask(
this.segmentProvider = new SegmentProvider(dataSource,
this.ioConfig.getInputSpec());
this.partitionConfigurationManager = new
PartitionConfigurationManager(this.tuningConfig);
this.segmentCacheManagerFactory = segmentCacheManagerFactory;
+
+ // Do not load any lookups in sub-tasks launched by compaction task,
unless transformSpec is present.
+ // If transformSpec is present, we will not modify the context so that the
sub-tasks can make the
+ // decision based on context values, loading all lookups by default.
+ // This is done to ensure backward compatibility since transformSpec can
reference lookups.
+ if (transformSpec == null) {
+ addToContextIfAbsent(LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE,
LookupLoadingSpec.Mode.NONE.toString());
+ }
Review Comment:
Shoudn't the else be all lookupLoading ?
##########
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 createFromContext(Map<String, Object>
context, LookupLoadingSpec defaultSpec)
+ {
+ if (context == null) {
+ return defaultSpec;
+ }
+
+ final Object lookupModeValue =
context.get(LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE);
+ if (lookupModeValue == null) {
+ return defaultSpec;
+ }
+
+ final LookupLoadingSpec.Mode lookupLoadingMode =
LookupLoadingSpec.Mode.valueOf(lookupModeValue.toString());
Review Comment:
Please add a test case for passing invalid lookup loading spec modes and
both values.
For eg mode: `NONE1`,`A ll`
and lookups to load are : `foo,bar`' , `foo,bar]` , `foo bar`
##########
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 createFromContext(Map<String, Object>
context, LookupLoadingSpec defaultSpec)
+ {
+ if (context == null) {
+ return defaultSpec;
+ }
+
+ final Object lookupModeValue =
context.get(LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE);
+ if (lookupModeValue == null) {
+ return defaultSpec;
+ }
+
+ final LookupLoadingSpec.Mode lookupLoadingMode =
LookupLoadingSpec.Mode.valueOf(lookupModeValue.toString());
Review Comment:
Also add a case insensitive test case for modes.
eg :
`None`, `aLL`, `all`
##########
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 createFromContext(Map<String, Object>
context, LookupLoadingSpec defaultSpec)
Review Comment:
Can we remove this default spec and have a context in the lookupLoadingSpec
module for the default ?
##########
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 createFromContext(Map<String, Object>
context, LookupLoadingSpec defaultSpec)
+ {
+ if (context == null) {
+ return defaultSpec;
+ }
+
+ final Object lookupModeValue =
context.get(LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE);
+ if (lookupModeValue == null) {
+ return defaultSpec;
+ }
+
+ final LookupLoadingSpec.Mode lookupLoadingMode =
LookupLoadingSpec.Mode.valueOf(lookupModeValue.toString());
Review Comment:
Error messages should instruct the user what to do.
--
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]
