kgyrtkirk commented on code in PR #16420:
URL: https://github.com/apache/druid/pull/16420#discussion_r1595033863
##########
server/src/test/java/org/apache/druid/server/lookup/cache/LookupLoadingSpecTest.java:
##########
@@ -59,4 +66,62 @@ public void testLoadingOnlyRequiredLookupsWithNullList()
DruidException exception = Assert.assertThrows(DruidException.class, () ->
LookupLoadingSpec.loadOnly(null));
Assert.assertEquals("Expected non-null set of lookups to load.",
exception.getMessage());
}
+
+ @MethodSource("provideParamsForTestGetSpecFromContext")
+ @ParameterizedTest
+ public void testGetLookupLoadingSpecFromContext(Map<String, Object> context,
LookupLoadingSpec defaultSpec, LookupLoadingSpec expectedSpec)
+ {
+ LookupLoadingSpec specFromContext =
LookupLoadingSpec.getSpecFromContext(context, defaultSpec);
+ Assert.assertEquals(expectedSpec.getMode(), specFromContext.getMode());
+ Assert.assertEquals(expectedSpec.getLookupsToLoad(),
specFromContext.getLookupsToLoad());
+ }
+
+ public static Collection<Object[]> provideParamsForTestGetSpecFromContext()
+ {
+ ImmutableSet<String> lookupsToLoad = ImmutableSet.of("lookupName1",
"lookupName2");
+ final ImmutableMap<String, Object> contextWithModeOnlyRequired =
ImmutableMap.of(
+ LookupLoadingSpec.CTX_LOOKUPS_TO_LOAD, new ArrayList<>(lookupsToLoad),
+ LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE,
LookupLoadingSpec.Mode.ONLY_REQUIRED);
+ final ImmutableMap<String, Object> contextWithModeNone = ImmutableMap.of(
+ LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE,
LookupLoadingSpec.Mode.NONE);
+ final ImmutableMap<String, Object> contextWithModeAll = ImmutableMap.of(
+ LookupLoadingSpec.CTX_LOOKUP_LOADING_MODE, LookupLoadingSpec.Mode.ALL);
+ final ImmutableMap<String, Object> contextWithoutLookupKeys =
ImmutableMap.of();
+
+ // Return params: <context, default LookupLoadingSpec, expected
LookupLoadingSpec>
+ Object[][] params = new Object[][]{
+ // Default spec is returned in the case of context not having the
lookup keys.
Review Comment:
I wonder if the intention was to name the set of config somehow; you could
use: `Named.of("default", ... )` to name parameters
##########
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)
+ {
+ 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());
+
+ if (lookupLoadingMode == LookupLoadingSpec.Mode.NONE) {
+ return LookupLoadingSpec.NONE;
+ } else if (lookupLoadingMode == LookupLoadingSpec.Mode.ALL) {
+ return LookupLoadingSpec.ALL;
+ } else if (lookupLoadingMode == LookupLoadingSpec.Mode.ONLY_REQUIRED) {
+ Collection<String> lookupsToLoad = (Collection<String>)
context.get(LookupLoadingSpec.CTX_LOOKUPS_TO_LOAD);
+ if (lookupsToLoad == null || lookupsToLoad.isEmpty()) {
+ throw InvalidInput.exception("Set of lookups to load cannot be %s for
mode[ONLY_REQUIRED].", lookupsToLoad);
+ }
Review Comment:
I wonder why its not valid to have it empty - if there are no lookups used;
the `ONLY_REQUIRED` should be harmless or not?
##########
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 see a few context keys here-and-there ; but I think it would be better to:
co-locate them somewhere
(or introduce an infra to be pluggable/checkable/etc)
Could these context keys be moved to `QueryContexts` instead (that class is
under `processing`) ?
--
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]
