Add support for selective loading of lookups in the task layer #16328

Akshat-Jain · 2024-04-24T10:00:51Z

Description

This PR adds support for loading a specified subset of lookups in the task layer via a new class named LookupLoadingSpec. It updates the LookupReferencesManager to support 3 modes:

Load all the lookups if LookupLoadingSpec.mode is ALL.
Don't load any lookups if LookupLoadingSpec.mode is NONE.
Load only the specified lookups if LookupLoadingSpec.mode is ONLY_REQUIRED. The lookups to load are consumed using LookupLoadingSpec#getLookupsToLoad().

This PR also updates the KillUnusedSegmentsTask to return an empty set of lookups to load, as this task has no dependency on any lookups. Future work would involve implementing this functionality for other tasks, depending on their dependency on lookups.

Test plan

I did some manual testing which involved running KillUnusedSegmentsTask with different implementations of getLookupsToLoad() method, to test the behavior of all the 3 aforementioned modes.
Added or updated unit tests to validate the changed functionality.

This PR has:

been self-reviewed.
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
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 is met.
added integration tests.
been tested in a test Druid cluster.

kfaraz · 2024-04-24T14:01:12Z

There is another PR #16266 which also addresses the same problem, but takes a slightly different approach.

cc: @cryptoe , @Akshat-Jain

kfaraz · 2024-04-24T14:14:01Z

As I see it, pros of this approach:

It doesn't require changing the way a CliPeon is launched.
Allows specific task implementations such as MsqControllerTask to give dynamic implementations of their getLookupsToLoad() methods, thus allowing them to load any list of lookups. Even with the other approach, this method would be needed at some point down the line.
Doesn't require a new enum.

Cons of this approach:

We have attached a special meaning to null. In other words, null here means Load Everything, which is not too great. This can always be remedied though by returning a wrapper object instead of just a List<String>. The wrapper object can have a boolean or enum flag to indicate the tristate: ALL, NONE, REQUIRED.
Based on this comment (Do not Load lookups on tasks that will not use them #13324 (comment)), it seems that MSQ workers know about the lookups they need to load only after they start running (or maybe I am reading it wrong). The approach in this PR injects the list of lookups to load at start-up itself. Is the list of lookups required for a worker included in its task payload?

cc: @cryptoe

server/src/main/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfig.java

Akshat-Jain · 2024-04-24T14:41:35Z

Is the list of lookups required for a worker included in its task payload?

@kfaraz IIUC, it's not. This PR only intends to add support for selective loading of lookups in the task layer, with task changes only for a sample task (KillUnusedSegmentsTask). The work for extracting the list of lookups and passing them for tasks that need lookups is pending, and I was thinking of it as my next step as a separate PR.

With that said, this should be applicable for both approaches.

IgorBerman · 2024-04-24T15:10:32Z

@Akshat-Jain Hi
do you mind to mark Compaction task not to load lookups as well please? same as you did for Kill task ?

Akshat-Jain · 2024-04-24T15:15:34Z

@IgorBerman Sure, I can do that! :)
Will add it first thing tomorrow!

kfaraz

Thanks for the changes, @Akshat-Jain ! Overall, the changes look good. I have left some minor suggestions.

indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java

...xing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java

server/src/test/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfigTest.java

server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java

kfaraz

Thanks for fixing up the tests!

Akshat-Jain · 2024-04-25T08:38:43Z

@IgorBerman Have updated CompactionTask to not load lookups as well.
cc: @kfaraz

indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

cryptoe · 2024-04-25T08:20:39Z

services/src/main/java/org/apache/druid/cli/CliPeon.java

+          @Provides
+          @LazySingleton
+          @Named(DataSourceTaskIdHolder.LOOKUPS_TO_LOAD_FOR_TASK)
+          public List<String> getLookupsToLoad(final Task task)


How does it work for indexers and mm less task system. Does that code go via cliPeon ? Maybe you can test locally and see how it goes.

Indexer is a single process, so it should always load all lookups, right? IIUC, the changes here would only bind the lookupsToLoad for peons, which is what we want.

Not too sure about MM-less system. The other PR had tweaked K8sTaskAdapter to handle it there.

I tried using Indexers. All lookups are loaded at the very beginning for Indexers. Changes of this PR don't affect the Indexer flow in any way.

For MM-less system, I'm not sure yet. @cryptoe Based on our earlier discussion, I have to setup the Kubernetes thing locally and try MM-less ingestion, and see what happens.

mmless runs CliPeon as the entrypoint so it should work as intended

indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java

indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java

...xing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java

indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java

server/src/test/java/org/apache/druid/server/lookup/cache/LookupLoadingSpecTest.java

cryptoe

Changes LGTM. Looking forward to results from MM less ingestion testing.

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java

kfaraz

LGTM

Akshat-Jain · 2024-04-29T07:51:28Z

Thanks @kfaraz and @cryptoe for the exhaustive review! 🙌

…e#16328) Changes: - Add `LookupLoadingSpec` to support 3 modes of lookup loading: ALL, NONE, ONLY_REQUIRED - Add method `Task.getLookupLoadingSpec()` - Do not load any lookups for `KillUnusedSegmentsTask`

…ayer (#16328) (#16394) Changes: - Add `LookupLoadingSpec` to support 3 modes of lookup loading: ALL, NONE, ONLY_REQUIRED - Add method `Task.getLookupLoadingSpec()` - Do not load any lookups for `KillUnusedSegmentsTask` Co-authored-by: Akshat Jain <akjn11@gmail.com>

Add support for selective loading of lookups in the task layer

449873b

github-actions bot added Area - Querying Area - Ingestion labels Apr 24, 2024

Fix checkstyle

f60927b

kfaraz mentioned this pull request Apr 24, 2024

feature-13324 contol loading lookups in peons #16266

Closed

10 tasks

kfaraz reviewed Apr 24, 2024

View reviewed changes

server/src/main/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfig.java Outdated Show resolved Hide resolved

kfaraz reviewed Apr 25, 2024

View reviewed changes

Akshat-Jain added 2 commits April 25, 2024 12:27

Address review comments

a731429

Fix checkstyle

33d0413

kfaraz reviewed Apr 25, 2024

View reviewed changes

server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java Outdated Show resolved Hide resolved

server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java Outdated Show resolved Hide resolved

Address review comments

a8dc8c3

kfaraz approved these changes Apr 25, 2024

View reviewed changes

Update CompactionTask to avoid loading any lookups

d6c4691

cryptoe reviewed Apr 25, 2024

View reviewed changes

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java Outdated Show resolved Hide resolved

Address review comments: Use POJO + Remove changes for CompactionTask

4fc0d13

Akshat-Jain force-pushed the selective-loading-of-lookups branch from e26a4e2 to 4fc0d13 Compare April 26, 2024 14:24

Akshat-Jain added 3 commits April 26, 2024 20:00

Minor cleanup

fceb436

Fix checkstyle

5bd36ee

Fix tests

62f6d01

kfaraz reviewed Apr 27, 2024

View reviewed changes

Akshat-Jain added 2 commits April 27, 2024 09:36

Address review comments

bd4b28d

Add LookupLoadingSpecTest

9a5e62e

github-advanced-security bot found potential problems Apr 27, 2024

View reviewed changes

server/src/test/java/org/apache/druid/server/lookup/cache/LookupLoadingSpecTest.java Fixed Show fixed Hide fixed

cryptoe approved these changes Apr 27, 2024

View reviewed changes

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java Show resolved Hide resolved

server/src/main/java/org/apache/druid/server/lookup/cache/LookupLoadingSpec.java Show resolved Hide resolved

kfaraz reviewed Apr 27, 2024

View reviewed changes

Akshat-Jain added 3 commits April 28, 2024 12:52

Address review comments

89dda07

Update ENUM in the Javadoc

c566afa

Update test method names to reflect the updated ENUM value

80827a3

kfaraz reviewed Apr 28, 2024

View reviewed changes

Akshat-Jain added 2 commits April 28, 2024 15:50

Address review comments

713053c

Use ImmutableSet instead of ImmutableList

0d08dc2

kfaraz approved these changes Apr 28, 2024

View reviewed changes

kfaraz merged commit 9d2cae4 into apache:master Apr 29, 2024
87 checks passed

cryptoe added this to the 30.0.0 milestone Apr 29, 2024

adarshsanjeev mentioned this pull request May 6, 2024

[Backport] Add support for selective loading of lookups in the task layer #16394

Merged

adarshsanjeev mentioned this pull request May 28, 2024

[DRAFT] 30.0.0 release notes #16505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for selective loading of lookups in the task layer #16328

Add support for selective loading of lookups in the task layer #16328

Akshat-Jain commented Apr 24, 2024 •

edited

Loading

kfaraz commented Apr 24, 2024

kfaraz commented Apr 24, 2024 •

edited

Loading

Akshat-Jain commented Apr 24, 2024

IgorBerman commented Apr 24, 2024

Akshat-Jain commented Apr 24, 2024

kfaraz left a comment

kfaraz left a comment

Akshat-Jain commented Apr 25, 2024

cryptoe Apr 25, 2024

kfaraz Apr 25, 2024

Akshat-Jain Apr 25, 2024

georgew5656 May 10, 2024

cryptoe left a comment

kfaraz left a comment

Akshat-Jain commented Apr 29, 2024

Add support for selective loading of lookups in the task layer #16328

Add support for selective loading of lookups in the task layer #16328

Conversation

Akshat-Jain commented Apr 24, 2024 • edited Loading

Description

Test plan

kfaraz commented Apr 24, 2024

kfaraz commented Apr 24, 2024 • edited Loading

Akshat-Jain commented Apr 24, 2024

IgorBerman commented Apr 24, 2024

Akshat-Jain commented Apr 24, 2024

kfaraz left a comment

Choose a reason for hiding this comment

kfaraz left a comment

Choose a reason for hiding this comment

Akshat-Jain commented Apr 25, 2024

cryptoe Apr 25, 2024

Choose a reason for hiding this comment

kfaraz Apr 25, 2024

Choose a reason for hiding this comment

Akshat-Jain Apr 25, 2024

Choose a reason for hiding this comment

georgew5656 May 10, 2024

Choose a reason for hiding this comment

cryptoe left a comment

Choose a reason for hiding this comment

kfaraz left a comment

Choose a reason for hiding this comment

Akshat-Jain commented Apr 29, 2024

Akshat-Jain commented Apr 24, 2024 •

edited

Loading

kfaraz commented Apr 24, 2024 •

edited

Loading