You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "IgorBerman (via GitHub)" <gi...@apache.org> on 2024/04/11 14:53:35 UTC
[PR] feature-13324 contol loading lookups in peons (druid)
IgorBerman opened a new pull request, #16266:
URL: https://github.com/apache/druid/pull/16266
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixes [#13324](https://github.com/apache/druid/issues/13324)
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Permits to disable lookups loading for some types of peons
for example compaction or kill tasks
this should improve memory footprint
e.g. for compaction we can use this memory for loading more rows into memory and thus improving throughput of compaction
I've followed same path as loadBroadcastSegments parameter works and reused LookupSerdeModule which already used in multiple services that doesn't need to load lookups
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### The fix adds doesntNeedLookups method to interface of task with default of false
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `Task` - added doesntNeedLookups with default implementation that returns false, i.e. most of peons will continue to load lookups
* `CompactionTask` - overrides it with true
* `CliPeon` - refers to the process parameter and loads relevant lookup module(which either loads lookups or not)
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573316083
##########
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:
maybe in additional to @nullable need to add empty c'tor to LookupNodeService ?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2068627010
@zachjsh Hi, maybe you can advice regarding org.apache.druid.indexing.overlord.ForkingTaskRunnerTest#testInvalidTaskContextJavaOptsArray and updating FAILED_TASK_COUNT & SUCCESSFUL_TASK_COUNT?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2069128643
@kfaraz can you tell me what I should do in addition for this PR to be merged?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz closed pull request #16266: feature-13324 contol loading lookups in peons
URL: https://github.com/apache/druid/pull/16266
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2068631600
run same test on master branch and it's failing as well, which is strange
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1572446136
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -363,7 +372,7 @@ public String getTaskIDFromTask(final Task task)
new IndexingServiceTuningConfigModule(),
new InputSourceModule(),
new ChatHandlerServerModule(properties),
- new LookupModule()
+ LookupLoadingMode.NONE == loadLookups ? new LookupSerdeModule() : new LookupModule()
Review Comment:
added log and private method. also added tests for it
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2068623141
I see that 1 test failing: org.apache.druid.indexing.overlord.ForkingTaskRunnerTest#testInvalidTaskContextJavaOptsArray
I've looked at it and for me it seems like some kind of race(meanwhile can't figure out how it's connected to what I've did)
I believe there is some inter-tests dependency
getWorkerSuccessfulTaskCount & getWorkerFailedTaskCount are using static variables
private static final AtomicLong FAILED_TASK_COUNT = new AtomicLong();
private static final AtomicLong SUCCESSFUL_TASK_COUNT = new AtomicLong();
both of them updated at
````
if (status.isSuccess()) {
SUCCESSFUL_TASK_COUNT.incrementAndGet();
} else {
FAILED_TASK_COUNT.incrementAndGet();
}
````
which doesn't happen when parsing of arguments failing here
````
List<String> taskJavaOptsArray = jsonMapper.convertValue(
task.getContextValue(ForkingTaskRunnerConfig.JAVA_OPTS_ARRAY_PROPERTY),
new TypeReference<List<String>>() {}
);
````
which throws exception and which is goes up to outer
````
catch (Throwable t) {
throw closer.rethrow(t);
}
````
i.e. those static variables are not incremented as part of failure to parse arguments(maybe should be fixed)
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2072023059
@IgorBerman , I have created #16323 to fix up the test for now so that the builds pass. Later, we can revisit the code and decide if we want to increment the failed task count in the case where the javaOptsArray is invalid.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573315829
##########
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:
actually @Nullable is the way to mark it optional...so I'm not sure why it doesn't work
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2067704719
@kfaraz thanks for the review! I think I've addressed all your comments besides binding of Lookup service. Tried your idea but seems compaction peon fails with this approach.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2074400442
@IgorBerman , I have merged #16323 . You can try to merge the latest master into your branch to fix the errors.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573315219
##########
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:
refactored to private method
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2072138694
thanks @kfaraz , I left 1 comment there
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2075156683
@kfaraz thanks :) I looked at it and it seems nice approach.
I think the other PR is more generic since it permits to control which lookups will be loaded. I haven't understood if it supports configuring it as some context arguments for task.
In our case it's always all or nothing, but I can imagine more involved environments.
For our usecase Compaction and Kill tasks are relevant and seems like it will be easy to support Compaction task as well.
Additional diff that I can think of is that if we are not going to load lookups current PR uses different guice module which might create lighter runtime(i.e. less beans e.g., but I'm not sure this is the case since I'm not too familiar with differences of those 2 modules really).
This is basically the other side of coin - to support higher flexibility when making decision a bit later(when everything is initialized already and only lookups are initialized) vs to make some decision during startup of task(CliPeon)
Personally I'm in favor for other PR seems so.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1572735589
##########
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 the `LookupNodeService` object 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.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573314626
##########
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:
tried this and got following error for compaction task
````
Exception in thread "main" java.lang.RuntimeException: java.lang.RuntimeException: com.google.inject.CreationException: Unable to create injector, see the following errors:
1) Could not find a suitable constructor in org.apache.druid.discovery.LookupNodeService. Classes must have either one (and only one) constructor annotated with @Inject or a zero-argument constructor that is not private.
at org.apache.druid.discovery.LookupNodeService.class(LookupNodeService.java:38)
while locating org.apache.druid.discovery.LookupNodeService
for the 26th parameter of org.apache.druid.indexing.common.TaskToolboxFactory.<init>(TaskToolboxFactory.java:165)
at org.apache.druid.cli.CliPeon.bindTaskConfigAndClients(CliPeon.java:506) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.cli.CliPeon$1)
1 error
at org.apache.druid.cli.CliPeon.run(CliPeon.java:421)
at org.apache.druid.cli.Main.main(Main.java:112)
Caused by: java.lang.RuntimeException: com.google.inject.CreationException: Unable to create injector, see the following errors:
````
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1574207696
##########
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:
No problem, we can try to address this in a follow up.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1572445610
##########
server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java:
##########
@@ -58,6 +59,7 @@ public List<? extends Module> getJacksonModules()
public void configure(Binder binder)
{
JsonConfigProvider.bind(binder, LookupModule.PROPERTY_BASE, LookupConfig.class);
+ binder.bind(LookupNodeService.class).toInstance(new LookupNodeService(LookupListeningAnnouncerConfig.DEFAULT_TIER));
Review Comment:
added
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ForkingTaskRunnerTest.java:
##########
@@ -537,6 +539,115 @@ ProcessHolder runTaskProcess(List<String> command, File logFile, TaskLocation ta
Assert.assertTrue(forkingTaskRunner.restore().isEmpty());
}
+ @Test
+ public void testTaskRun_doesnt_pass_loadLookups_Argument() throws Exception
Review Comment:
renamed
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1569574852
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -379,6 +379,11 @@ public TaskStatus call()
command.add("true");
}
+ if (task.doesntNeedLookups()) {
+ command.add("--doesntNeedLookups");
Review Comment:
renamed
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573315109
##########
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:
I'm not too familiar with Guice(using spring boot at work) so might be there is other option to have Optional dependency?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2074466389
thanks @kfaraz , updated my branch with upstream
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1564848772
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -331,4 +331,9 @@ static TaskInfo<TaskIdentifier, TaskStatus> toTaskIdentifierInfo(TaskInfo<Task,
taskInfo.getTask().getMetadata()
);
}
+
+ default boolean doesntNeedLookups()
Review Comment:
thanks @kfaraz for the comments.
Makes sense to me.
I'll ping you when your suggestions will be addressed.
Also here is something that I've discovered lately and if you have some suggestions/ideas please share them: I've tried to start nano cluster with my changes and discovered that it's not that simple. TaskToolbox injects LookupNodeService, i.e. currently it's not possible to remove it without additional refactoring, so I'm trying to convert it to "Lazy" one by injecting Providing<LookupNodeService> instead so that LookupSerdeModule could create this Provider as well
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1572449207
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -185,6 +187,13 @@ public class CliPeon extends GuiceRunnable
@Option(name = "--loadBroadcastSegments", title = "loadBroadcastSegments", description = "Enable loading of broadcast segments")
public String loadBroadcastSegments = "false";
+
+ /**
+ * If set to true then lookups won't be loaded
Review Comment:
updated
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573314849
##########
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:
added
##########
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:
replaced with assertThrows
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2068812139
> run same test on master branch and it's failing as well, which is strange
Yeah, oddly enough, for me, the test fails only when I run it individually. When I run all the tests in this class from the IDE together, it passes. It is most likely because the failed/success task counts are static.
A test that runs before this one must be setting the failed count to 1, which is being verified in this one too.
When run by itself, this test is not able to set the failed count to 1.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2076731272
> I haven't understood if it supports configuring it as some context arguments for task.
The author intends to create that as a follow up PR.
> Personally I'm in favor for other PR seems so.
Thanks, @IgorBerman .
Closing this PR in favour of #16328 .
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2063324441
run nano cluster locally and verified that all services started, ingestion from kafka works and compaction also able to work.
compaction task gets argument of
`2024-04-18T08:28:44,500 INFO [main] org.apache.druid.cli.CliPeon - * sun.java.command: org.apache.druid.cli.Main internal peon var/druid/task/slot1/compact_my_ds_2024-04-18T08:28:42.514Z 1 --loadLookups NONE`
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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 small javadoc here as well.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1573329870
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ForkingTaskRunnerTest.java:
##########
@@ -537,6 +539,78 @@ ProcessHolder runTaskProcess(List<String> command, File logFile, TaskLocation ta
Assert.assertTrue(forkingTaskRunner.restore().isEmpty());
}
+ @Test
+ public void testTaskRunDoesntPassLoadLookupsArgument() throws Exception
+ {
+ verifyTaskRunLoadLookupsArgument(LookupLoadingMode.ALL);
+ }
+
+ @Test
+ public void testTaskRunPassLoadLookupsArgument() throws Exception
+ {
+ verifyTaskRunLoadLookupsArgument(LookupLoadingMode.NONE);
+ }
+
+ private void verifyTaskRunLoadLookupsArgument(LookupLoadingMode lookupLoadingMode) throws Exception
+ {
+ ObjectMapper mapper = new DefaultObjectMapper();
+ Task task = new NoopTask(null, null, null, 0, 0, null) {
+ @Override
+ public LookupLoadingMode loadLookups()
+ {
+ return lookupLoadingMode;
+ }
+ };
+ File file = temporaryFolder.newFolder();
+ TaskConfig taskConfig = makeDefaultTaskConfigBuilder()
+ .setBaseTaskDir(file.toString())
+ .build();
+ final WorkerConfig workerConfig = new WorkerConfig();
+ ForkingTaskRunner forkingTaskRunner = new ForkingTaskRunner(
+ new ForkingTaskRunnerConfig(),
+ taskConfig,
+ workerConfig,
+ new Properties(),
+ new NoopTaskLogs(),
+ mapper,
+ new DruidNode("middleManager", "host", false, 8091, null, true, false),
+ new StartupLoggingConfig(),
+ TaskStorageDirTracker.fromConfigs(workerConfig, taskConfig)
+ )
+ {
+ @Override
+ ProcessHolder runTaskProcess(List<String> command, File logFile, TaskLocation taskLocation) throws IOException
+ {
+ if (lookupLoadingMode == LookupLoadingMode.NONE) {
+ Assert.assertTrue(command.containsAll(ImmutableSet.of("--loadLookups", LookupLoadingMode.NONE.name())));
+ } else {
+ Assert.assertFalse(command.containsAll(ImmutableSet.of("--loadLookups", LookupLoadingMode.NONE.name())));
Review Comment:
Nit:
This assertion should be broken up to ensure that neither of the values are present.
```suggestion
Assert.assertFalse(command.contains("--loadLookups"));
Assert.assertFalse(command.contains(LookupLoadingMode.NONE.name()));
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupLoadingMode.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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:
+ * <ul>
+ * <li>ALL - represents regular mode of loading all lookups </li>
+ * <li>NONE - represents mode when no lookups will be loaded </li>
+ * <li>ON_DEMAND - currently not supported and will be used in the future </li>
+ * </ul>
+ */
+public enum LookupLoadingMode
+{
+ NONE, ALL, ON_DEMAND
Review Comment:
Should we comment out the `ON_DEMAND` value right now so that no tasks end up using it causing an exception in `CliPeon`? We can uncomment it when we have an actual implementation for it.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2066686337
@kfaraz Hi, I think I addressed all of your comments. Also hopefully fixed coverage.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "IgorBerman (via GitHub)" <gi...@apache.org>.
IgorBerman commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1569575108
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -331,4 +331,9 @@ static TaskInfo<TaskIdentifier, TaskStatus> toTaskIdentifierInfo(TaskInfo<Task,
taskInfo.getTask().getMetadata()
);
}
+
+ default boolean doesntNeedLookups()
Review Comment:
renamed interface @kfaraz
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1571682209
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -185,6 +187,13 @@ public class CliPeon extends GuiceRunnable
@Option(name = "--loadBroadcastSegments", title = "loadBroadcastSegments", description = "Enable loading of broadcast segments")
public String loadBroadcastSegments = "false";
+
+ /**
+ * If set to true then lookups won't be loaded
Review Comment:
This javadoc needs to be updated.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java:
##########
@@ -58,6 +59,7 @@ public List<? extends Module> getJacksonModules()
public void configure(Binder binder)
{
JsonConfigProvider.bind(binder, LookupModule.PROPERTY_BASE, LookupConfig.class);
+ binder.bind(LookupNodeService.class).toInstance(new LookupNodeService(LookupListeningAnnouncerConfig.DEFAULT_TIER));
Review Comment:
Please add a comment here on why this needs to be done.
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -363,7 +372,7 @@ public String getTaskIDFromTask(final Task task)
new IndexingServiceTuningConfigModule(),
new InputSourceModule(),
new ChatHandlerServerModule(properties),
- new LookupModule()
+ LookupLoadingMode.NONE == loadLookups ? new LookupSerdeModule() : new LookupModule()
Review Comment:
A log line which indicates the chosen lookup loading mode and the expected effects would also be helpful.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupLoadingMode.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * 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;
+
+public enum LookupLoadingMode
Review Comment:
Please add a javadoc for this.
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -363,7 +372,7 @@ public String getTaskIDFromTask(final Task task)
new IndexingServiceTuningConfigModule(),
new InputSourceModule(),
new ChatHandlerServerModule(properties),
- new LookupModule()
+ LookupLoadingMode.NONE == loadLookups ? new LookupSerdeModule() : new LookupModule()
Review Comment:
Maybe put this logic in a private method `getLookupModule()` and add a javadoc there explaining what we are doing.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ForkingTaskRunnerTest.java:
##########
@@ -537,6 +539,115 @@ ProcessHolder runTaskProcess(List<String> command, File logFile, TaskLocation ta
Assert.assertTrue(forkingTaskRunner.restore().isEmpty());
}
+ @Test
+ public void testTaskRun_doesnt_pass_loadLookups_Argument() throws Exception
Review Comment:
The other tests in this class don't use the underscore style of naming tests. Please adhere to the existing style in this class if possible.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16266:
URL: https://github.com/apache/druid/pull/16266#issuecomment-2075056446
@IgorBerman , FYI, there is another PR https://github.com/apache/druid/pull/16328 which tackles the same problem as this one. Let me know what you think about it.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feature-13324 contol loading lookups in peons (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1564837520
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -379,6 +379,11 @@ public TaskStatus call()
command.add("true");
}
+ if (task.doesntNeedLookups()) {
+ command.add("--doesntNeedLookups");
Review Comment:
The parameter should be called `--loadLookups` instead, and should take values `all`, `none`, `onDemand`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -331,4 +331,9 @@ static TaskInfo<TaskIdentifier, TaskStatus> toTaskIdentifierInfo(TaskInfo<Task,
taskInfo.getTask().getMetadata()
);
}
+
+ default boolean doesntNeedLookups()
Review Comment:
The method should rather be called `needsLookups()` or `loadLookups()`. There is no point naming this method in the negative.
Also, since we want to extend this feature in the future to be able to load lookups on demand, I think we should return an enum here with possible values `NONE`, `ALL`, `ON_DEMAND`.
For the time-being, the behaviour for `ALL` and `ON_DEMAND` would be the same. But later, `ON_DEMAND` can be optimized to load only required lookups.
So, overall:
```java
default LookupLoadingMode needsLookups()
{
return LookupLoadingMode.ALL;
}
```
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org