kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1572731837
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -331,4 +332,9 @@ static TaskInfo<TaskIdentifier, TaskStatus>
toTaskIdentifierInfo(TaskInfo<Task,
taskInfo.getTask().getMetadata()
);
}
+
+ default LookupLoadingMode loadLookups()
Review Comment:
Need a javadoc here.
##########
services/src/test/java/org/apache/druid/cli/CliPeonTest.java:
##########
@@ -73,6 +76,35 @@ public void testCliPeonK8sANdWorkerIsK8sMode() throws
IOException
Assert.assertNotNull(runnable.makeInjector());
}
+ @Test
+ public void testResolveLookupModule() throws IOException
+ {
+ File file = temporaryFolder.newFile("task.json");
+ FileUtils.write(file, "{\"type\":\"noop\"}", StandardCharsets.UTF_8);
+ FakeCliPeon peon = new FakeCliPeon(file.getParent(), "k8sAndWorker");
+ Assert.assertTrue(peon.resolveLookupModule() instanceof LookupModule);
+ }
+
+ @Test
+ public void testResolveSerdeLookupModule() throws IOException
+ {
+ File file = temporaryFolder.newFile("task.json");
+ FileUtils.write(file, "{\"type\":\"noop\"}", StandardCharsets.UTF_8);
+ FakeCliPeon peon = new FakeCliPeon(file.getParent(), "k8sAndWorker");
+ peon.loadLookups = LookupLoadingMode.NONE;
+ Assert.assertTrue(peon.resolveLookupModule() instanceof LookupSerdeModule);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
Review Comment:
Please use `Assert.assertThrows()` instead.
##########
services/src/test/java/org/apache/druid/cli/CliPeonTest.java:
##########
@@ -73,6 +76,35 @@ public void testCliPeonK8sANdWorkerIsK8sMode() throws
IOException
Assert.assertNotNull(runnable.makeInjector());
}
+ @Test
+ public void testResolveLookupModule() throws IOException
Review Comment:
Thanks for adding the tests!
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -556,6 +565,28 @@ static void configureIntermediaryData(Binder binder)
shuffleClientBiddy.addBinding("deepstore").to(DeepStorageShuffleClient.class).in(LazySingleton.class);
}
+
+ /**
+ * Resolves which lookup module to use in current process.
+ *
+ * <p>Currently we support 2 different modes of loading lookups: either
loading them with ({@link LookupModule}) or not loading lookups( {@link
LookupSerdeModule } </p>
+ *
+ * @return lookup module based on loadLookups argument
Review Comment:
```suggestion
* <p>
* There are two modes of loading lookups:
* <ol>
* <li>loadLookups = ALL. The method returns a {@link LookupModule} which
loads all lookups on startup.</li>
* <li>loadLookups = NONE. The method returns a {@link LookupSerdeModule}
which does not load any lookup but can still parse queries that employ
lookups.</p>
* </ol>
*
* @return Lookup module to use for this process based on loadLookups
argument.
```
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -556,6 +565,28 @@ static void configureIntermediaryData(Binder binder)
shuffleClientBiddy.addBinding("deepstore").to(DeepStorageShuffleClient.class).in(LazySingleton.class);
}
+
+ /**
+ * Resolves which lookup module to use in current process.
+ *
+ * <p>Currently we support 2 different modes of loading lookups: either
loading them with ({@link LookupModule}) or not loading lookups( {@link
LookupSerdeModule } </p>
+ *
+ * @return lookup module based on loadLookups argument
+ */
+ Module resolveLookupModule()
+ {
+ switch (loadLookups) {
+ case NONE:
+ log.info("loadLookups mode is %s, no lookups will be loaded",
loadLookups);
Review Comment:
```suggestion
log.info("loadLookups mode is [%s], no lookups will be loaded.",
loadLookups);
```
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -556,6 +565,28 @@ static void configureIntermediaryData(Binder binder)
shuffleClientBiddy.addBinding("deepstore").to(DeepStorageShuffleClient.class).in(LazySingleton.class);
}
+
+ /**
+ * Resolves which lookup module to use in current process.
+ *
+ * <p>Currently we support 2 different modes of loading lookups: either
loading them with ({@link LookupModule}) or not loading lookups( {@link
LookupSerdeModule } </p>
+ *
+ * @return lookup module based on loadLookups argument
+ */
+ Module resolveLookupModule()
+ {
+ switch (loadLookups) {
+ case NONE:
+ log.info("loadLookups mode is %s, no lookups will be loaded",
loadLookups);
+ return new LookupSerdeModule();
+ case ALL:
+ log.info("loadLookups mode is %s, lookups will be loaded",
loadLookups);
Review Comment:
```suggestion
log.info("loadLookups mode is [%s], lookups will be loaded.",
loadLookups);
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupLoadingMode.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+/**
+ * This enum describes different modes of loading lookups
+ * <p>Each task might define if it needs to load lookups or not {@link
org.apache.druid.indexing.common.task.Task#loadLookups}
+ * <p>Values:
+ * <li>ALL - represents regular mode of loading all lookups </li>
Review Comment:
List items might need to be inside a `<ul></ul>` tag.
##########
extensions-contrib/kubernetes-overlord-extensions/src/test/java/org/apache/druid/k8s/overlord/taskadapter/K8sTaskAdapterTest.java:
##########
@@ -272,6 +275,123 @@ public PodSpec getSpec()
);
}
+
+ @Test
+ public void fromTask_doesnt_pass_loadLookups_Argument() throws IOException
+ {
+ final PodSpec podSpec = K8sTestUtils.getDummyPodSpec();
+ TestKubernetesClient testClient = new TestKubernetesClient(client)
+ {
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T> T executeRequest(KubernetesExecutor<T> executor) throws
KubernetesResourceNotFoundException
+ {
+ return (T) new Pod()
+ {
+ @Override
+ public PodSpec getSpec()
+ {
+ return podSpec;
+ }
+ };
+ }
+ };
+
+ KubernetesTaskRunnerConfig config = KubernetesTaskRunnerConfig.builder()
+
.withNamespace("test")
+ .build();
+ K8sTaskAdapter adapter = new SingleContainerTaskAdapter(
+ testClient,
+ config,
+ taskConfig,
+ startupLoggingConfig,
+ node,
+ jsonMapper,
+ taskLogs
+ );
+ Task task = new NoopTask(
+ "id",
+ "id",
+ "datasource",
+ 0,
+ 0,
+ ImmutableMap.of("context", RandomStringUtils.randomAlphanumeric((int)
DruidK8sConstants.MAX_ENV_VARIABLE_KBS * 20))
+ );
+ Job job = adapter.fromTask(task);
+ //Verify that --loadLookups NONE shouldn't be passed
+ Set<String> arguments = Arrays.stream(job.getSpec()
+ .getTemplate()
+ .getSpec()
+ .getContainers()
+ .get(0)
+ .getArgs()
+ .get(0).split("
")).collect(Collectors.toSet());
+
+ Assert.assertFalse(arguments.containsAll(ImmutableSet.of("--loadLookups",
LookupLoadingMode.NONE.name())));
+ }
+
+
+ @Test
+ public void fromTask_passes_loadLookups_Argument() throws IOException
Review Comment:
Seems like a lot of code can be commoned out between these two tests. You
can either merge these into a single test where task1 passes the argument and
task2 doesn't, or use a common private method that is called by both the tests.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java:
##########
@@ -58,6 +59,9 @@ public List<? extends Module> getJacksonModules()
public void configure(Binder binder)
{
JsonConfigProvider.bind(binder, LookupModule.PROPERTY_BASE,
LookupConfig.class);
+ //Even though LookupSerdeModule will be used for processes that are not
loading lookups, we need to bind LookupNodeService,
+ //so that TaskToolboxFactory and TaskToolbox will get their dependency.
Binding it won't load lookups.
Review Comment:
Hmm, I suppose we could have just made this dependency `@Nullable` in
`TaskToolboxFactory`.
The only usage is to build a `DruidDiscoveryNode` where this is used to
populate the services map.
From the point of service discovery, I think it would be more appropriate to
just not add the lookup node service to the map rather than declaring a dummy
node service.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java:
##########
@@ -58,6 +59,9 @@ public List<? extends Module> getJacksonModules()
public void configure(Binder binder)
{
JsonConfigProvider.bind(binder, LookupModule.PROPERTY_BASE,
LookupConfig.class);
+ //Even though LookupSerdeModule will be used for processes that are not
loading lookups, we need to bind LookupNodeService,
+ //so that TaskToolboxFactory and TaskToolbox will get their dependency.
Binding it won't load lookups.
Review Comment:
Nit:
```suggestion
// Even though LookupSerdeModule will be used for processes that are not
loading lookups, we need to bind LookupNodeService,
// so that TaskToolboxFactory and TaskToolbox will get their dependency.
Binding it won't load lookups.
```
--
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
