Enable querying entirely cold datasources by findingrish · Pull Request #16676 · apache/druid

findingrish · 2024-07-01T05:28:55Z

Problem

Currently, datasource schema doesn’t include columns from cold segments. This makes it impossible to query entirely cold datasource.

Approach

Mechanism to backfill schema for cold segments in the metadata database. Note, that this is required only for segments created prior to enabling CentralizedDatasourceSchema feature.
Update datasource schema building logic on the Coordinator to include schema from cold segments.
Make Brokers aware of entirely cold datasource.

Backfill schema for cold segments

Leverage the existing schema backfill flow added as part of CentralizedDatasourceSchema feature. Users are supposed to manually load the cold segments by making their replication factor as 1 and once the schema is backfilled (can be verified from the metadata database) they can unload the segment.

Handling entirely cold datasource

The problem with cold datasource is that Broker just doesn’t know about the datasource if none of the segment are available. So, the datasource wouldn’t even appear on the console for querying.
We need a way for the Brokers to be aware of cold datasource, so that it can fetch its schema from the Coordinator.

Currently, brokers request schema for available datasources from Coordinator in each refresh cycle.
Brokers now poll set of used datasources from the Coordinator first and then request their schema from the Coordinator.

Once Broker has schema for Cold datasources, it will show up in the console and become available for querying.

Key changes

CoordinatorSegmentMetadataCache
- It runs a scheduled thread to fetch used segments and build datasource schema from cold segments. Hot and cold schema is merged when datasource schema is queried.
BrokerSegmentMetadataCache
- The refresh condition is slightly updated, refresh is executed in each cycle if the feature is enabled.
- The refresh logic is also updated to poll used datasources from the Coordinator. This way Broker can fetch cold datasource schema.

Release Notes

CentralizedDatasourceSchema feature needs to be enabled in order to query entirely cold datasources, also it would enable querying on columns only present in cold segments.

This PR has:

…efresh on each cycle

kfaraz

This PR has some refactors which should be tackled separately, in order to facilitate a smoother review of the core changes here.

kfaraz · 2024-07-04T04:17:08Z

+    return datasourceToUnavailableSegments;
+  }
+
+  public Object2IntMap<String> getDatasourceToDeepStorageQueryOnlySegmentCount()


@findingrish , this seems like the only new method that has been added here.
Please remove this new class SegmentReplicationStatusManager and move the code back toDruidCoordinator.

If a refactor is required, please do it in a separate PR.
This PR should focus only on the required changes.

Correction: It seems that this method had already existed too.
@findingrish , is there any new code in SegmentReplicationStatusManager?

@kfaraz There is no new code in SegmentReplicationStatusManager. The reason for refactoring was a cyclic dependency between CoordinatorSegmentMetadataCache and DruidCoordinator while trying to use DruidCoordinator#getSegmentReplicationFactor.

I will raise a separate PR for the refactor.

Okay. Can you share some more details on how the cyclic dependency is coming into picture?

Currently DruidCoordinator has a dependency on CoordinatorSegmentMetadataCache, for this patch I need to use DruidCoordinator#getSegmentReplicationFactor in CoordinatorSegmentMetadataCache which is resulting in cyclic dependency.

As a solution, I have refactored DruidCoordinator to separate out the code which updates segmentReplicationStatus and broadcastSegments.

Let me know if this solution makes sense.

@findingrish , you could just expose a method updateSegmentReplicationStatus() on CoordinatorSegmentMetadataCache. Call this method from DruidCoordinator.UpdateReplicationStatus.run() where we update broadcastSegments and segmentReplicationStatus.

Let me know if this works for you.

Yeah, this approach would work for me.
However, it seems bit odd that DruidCoordinator.UpdateReplicationStatus has to additionally update state in some other class, ideally the consumer CoordinatorSegmentMetadataCache should be pulling this information?

Is there a reason to avoid the refactor work?

Is there a reason to avoid the refactor work?

Yes, the dependencies are already all over the place which makes the code less readable and also complicates testing. A refactor is needed here but it would have to be thought through a little.

However, it seems bit odd that DruidCoordinator.UpdateReplicationStatus has to additionally update state in some other class,

Not really, you can think of the DruidCoordinator (or rather the UpdateReplicationStatus duty in this case) as sending a notification to the CoordinatorSegmentMetadatCache saying that the segment replication status has been updated. The DruidCoordinator already sends notification to the metadata cache about leadership status, this is another notification in the same vein.

Yes, the dependencies are already all over the place which makes the code less readable and also complicates testing. A refactor is needed here but it would have to be thought through a little.

Yes, this makes sense. DruidCoordinator refactoring would need more thought.

Thanks for the suggestion, I will update the patch.

cryptoe

Left some comments.

cryptoe · 2024-07-05T03:56:25Z

+  /**
+   * Retrieves list of used datasources.
+   */
+  ListenableFuture<Set<String>> fetchUsedDataSources();


Please add the definition of used data sources here.

I have updated the method name to fetchDatasourcesWithUsedSegments to make it more understandable.
I don't think we need to document about what used segments means, since it is widely referred in the code and docs. For example, here https://druid.apache.org/docs/latest/api-reference/data-management-api/#mark-a-single-segment-as-used.

cryptoe · 2024-07-05T03:57:41Z

+   * It contains schema for datasources with atleast 1 available segment.
   */
-  protected final ConcurrentMap<String, T> tables = new ConcurrentHashMap<>();
+  protected final ConcurrentHashMap<String, T> tables = new ConcurrentHashMap<>();


Nit: Just wondering what specific hashMapMethods are you using which required this change.

I started using computeIfAbsent method. The explanation is captured here https://github.com/code-review-checklists/java-concurrency/blob/master/README.md#chm-type.

cryptoe · 2024-07-05T03:58:59Z

+    coldScehmaExec = Executors.newSingleThreadScheduledExecutor(
+        new ThreadFactoryBuilder()
+            .setNameFormat("DruidColdSchema-ScheduledExecutor-%d")
+            .setDaemon(true)


Why is this a demon thread ?

I will update, we don't need a daemon thread here.

cryptoe · 2024-07-05T04:04:04Z

      cacheExecFuture = cacheExec.submit(this::cacheExecLoop);
+      coldSchemaExecFuture = coldScehmaExec.schedule(
+          this::coldDatasourceSchemaExec,
+          coldSchemaExecPeriodMillis,


Is there a specific reason to undocumented these properties.
Do we have any metrics which tell us the performance of these executor service in terms of number of cold segments back filed ?

Do we have any metrics which tell us the performance of these executor service in terms of number of cold segments back filed

We are not backfilling segment here. It is just looping over the segments, identifying cold segment and building their schema.
If the datasource schema is updated it is logged.

Since this exec iterates over all the segments, what things do we have to figure out how much time it took for execution?
Should we publish some summary stats to increase operability.

Makes sense. I am logging details if the execution duration is greater than 50 seconds.

cryptoe · 2024-07-05T04:06:53Z

+    coldSchemaTable.keySet().retainAll(dataSources);
+  }
+
+  private RowSignature mergeHotAndColdSchema(RowSignature hot, RowSignature cold)


I am very surprised you need a new method here. There should be existing logic which does this no ?

I can refactor this a bit to have a single method for merging the RowSignature.

cryptoe · 2024-07-05T06:35:37Z

+    }
+
+    // remove any stale datasource from the map
+    coldSchemaTable.keySet().retainAll(dataSources);


Do we have a test case for this ?

Yes, in CoordinatorSegmentMetadataCacheTest#testColdDatasourceSchema_verifyStaleDatasourceRemoved.

kfaraz · 2024-07-08T04:03:58Z

+    );
+
+    SegmentReplicaCount segmentReplicaCount = new SegmentReplicaCount();
+    segmentReplicaCount.setRequired(0, 0);


Wouldn't the values already be zero in a fresh instance?

Right, I was trying to be explicit about setting it to 0, so that it is clear in the test that the given segment is unavailable.

You can just add a comment to that effect and/or use test method names that clarify that point.
Invoking this method requires making it public which doesn't really seem necessary since it is only going to be used in this test.

Makes sense. Updated.

asdf2014 · 2024-07-08T07:35:20Z

  /**
   * Map of datasource and generic object extending DataSourceInformation.
   * This structure can be accessed by {@link #cacheExec} and {@link #callbackExec} threads.
+   * It contains schema for datasources with atleast 1 available segment.


Suggested change

* It contains schema for datasources with atleast 1 available segment.

* It contains schema for datasources with at least 1 available segment.

cryptoe · 2024-07-09T06:06:19Z

+    coldSchemaTable.keySet().retainAll(dataSources);
+
+    if (stopwatch.millisElapsed() > COLD_SCHEMA_SLOWNESS_THRESHOLD_MILLIS) {
+      log.info("Cold schema processing was slow, taking [%d] millis. "


Else for this should be debug.

cryptoe · 2024-07-11T07:15:50Z

+    coldSchemaTable.keySet().retainAll(dataSourceWithColdSegmentSet);
+
+    String executionStatsLog = StringUtils.format(
+        "Cold schema processing was slow, taking [%d] millis. "


Suggested change

"Cold schema processing was slow, taking [%d] millis. "

"Cold schema processing took [%d] millis. "

cryptoe · 2024-07-11T07:17:22Z

+
+    String executionStatsLog = StringUtils.format(
+        "Cold schema processing was slow, taking [%d] millis. "
+        + "Processed [%d] datasources, [%d] segments & [%d] datasourceWithColdSegments.",


Suggested change

+ "Processed [%d] datasources, [%d] segments & [%d] datasourceWithColdSegments.",

+ "Processed total [%d] datasources, [%d] segments. Found [%d] datasources with cold segments.",

Add ability to query entirely cold datasources.

rishabh singh added 5 commits March 14, 2024 16:49

Fix build

ca33d8b

Merge branch 'master' of github.com:findingrish/druid

1abba25

temp changes

033c420

Coordinator changes to process schema for cold segments

96e77c1

Changes in the broker metadata cache to fetch used datasources schema

1739dce

github-actions Bot added the Area - Querying label Jul 1, 2024

rishabh singh added 4 commits July 2, 2024 10:22

Refactor DruidCoordinator, broker metadata cache changes to execute r…

6ed0b8b

…efresh on each cycle

Fix tests

966f5ac

Merge remote-tracking branch 'upstream/master' into cold_ds_schema

d586a8e

Update MetadataResourceTest

b72b822

github-advanced-security AI found potential problems Jul 2, 2024

View reviewed changes

Comment thread server/src/main/java/org/apache/druid/segment/metadata/AbstractSegmentMetadataCache.java Fixed

rishabh singh added 5 commits July 2, 2024 14:28

minor changes

ad58f9b

Fix refresh condition on Broker, add UTs

2944959

Fix test

b09df54

Merge remote-tracking branch 'upstream/master' into cold_ds_schema

254cea2

minor code changes, add test

25d23c6

kfaraz requested changes Jul 4, 2024

View reviewed changes

cryptoe reviewed Jul 5, 2024

View reviewed changes

rishabh singh added 2 commits July 8, 2024 00:10

revert changes

7169b6d

Fix test

89e2d64

kfaraz reviewed Jul 8, 2024

View reviewed changes

rishabh singh added 2 commits July 8, 2024 11:27

checkstyle

ba380ec

Update docs

d3c112c

asdf2014 requested changes Jul 8, 2024

View reviewed changes

rishabh singh added 4 commits July 8, 2024 16:16

review comments

debcb42

Doc changes

479382c

Update threshold for logging cold schema processing stats

87113ca

Minor changes

2a66ae1

cryptoe reviewed Jul 9, 2024

View reviewed changes

add debug logging

695ba92

rishabh singh added 6 commits July 10, 2024 11:28

Merge hot and cold schema only while querying datasource schema

a265cf0

Update tests

8182f39

Update docs

766082e

Merge remote-tracking branch 'upstream/master' into cold_ds_schema

2bea1fb

Update test

8582f5d

Update tests wiht SegmentReplicationStatus mock

7ed2d96

cryptoe approved these changes Jul 11, 2024

View reviewed changes

rishabh singh added 3 commits July 11, 2024 17:55

Merge remote-tracking branch 'upstream/master' into cold_ds_schema

47bf4ab

Minor change

8fb0a04

Minor change

87a0926

cryptoe merged commit 6410453 into apache:master Jul 15, 2024

317brian mentioned this pull request Aug 5, 2024

docs: update query from deepstorage segment requirement #16842

Merged

1 task

sreemanamala pushed a commit to sreemanamala/druid that referenced this pull request Aug 6, 2024

Enable querying entirely cold datasources (apache#16676)

4650462

Add ability to query entirely cold datasources.

findingrish mentioned this pull request Aug 9, 2024

[bugfix] Run cold schema refresh thread periodically #16873

Merged

10 tasks

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

kfaraz mentioned this pull request Oct 11, 2024

[DRAFT] 31.0.0 Release Notes #17332

Closed

	* It contains schema for datasources with atleast 1 available segment.
	* It contains schema for datasources with at least 1 available segment.

	"Cold schema processing was slow, taking [%d] millis. "
	"Cold schema processing took [%d] millis. "

	+ "Processed [%d] datasources, [%d] segments & [%d] datasourceWithColdSegments.",
	+ "Processed total [%d] datasources, [%d] segments. Found [%d] datasources with cold segments.",

Conversation

findingrish commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Approach

Backfill schema for cold segments

Handling entirely cold datasource

Key changes

Release Notes

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findingrish Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findingrish Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

findingrish commented Jul 1, 2024 •

edited

Loading

findingrish Jul 4, 2024 •

edited

Loading

kfaraz Jul 5, 2024 •

edited

Loading

findingrish Jul 8, 2024 •

edited

Loading