Add feature to automatically remove datasource metadata based on retention period by maytasm · Pull Request #11227 · apache/druid

maytasm · 2021-05-10T07:08:54Z

Add feature to automatically remove datasource metadata based on retention period

Description

We currently already have tasklog auto cleanup (#3677) and audit logs auto cleanup (#11084). This PR adds a similar auto cleanup based on duration (time to retained) but for the datasource metadata table to auto clean up datasource that is no longer active -- meaning that the datasource does not have active supervisor running (Note: datasource metadata only exists for datasource created from supervisor).

This is useful when Druid user has a high churn of task / datasource in a short amount of time causing the metadata store size to grow uncontrollably.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
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.

suneet-s · 2021-05-10T18:04:29Z

+              )
+          );
+          for (String datasourceMetadataInDb : datasources) {
+            if (!excludeDatasources.contains(datasourceMetadataInDb)) {


possible NPE - excludeDatasources is marked as nullable in the function definition

excludeDatasources should be @NotNull. Fixed

suneet-s · 2021-05-10T18:07:15Z

+            .mapTo(String.class)
+            .list()
+    );
+    return connector.getDBI().withHandle(


What happens if an exception is thrown while trying to delete the datasources? withHandle will throw a CallbackFailedException - is this handled somewhere else in the code?

Added try catch block

suneet-s · 2021-05-10T18:11:13Z

+        handle -> {
+          final PreparedBatch batch = handle.prepareBatch(
+              StringUtils.format(
+                  "DELETE FROM %1$s WHERE dataSource = :dataSource AND created_date < '%2$s'",


Why did you choose to build the delete statements one at a time instead of doing a batch delete?

I think we could encapsulate the excludeDatasources logic in a where clause of this delete statement instead.

Something like DELETE FROM datasources where created_date < "date" and datasource not in ("excludeDataSources")

This is to prevent the IN clause being too large

Also moved the filtering of the datasources to outside the handle block.

suneet-s · 2021-05-10T18:17:16Z

+    if ((lastKillTime + period) < System.currentTimeMillis()) {
+      lastKillTime = System.currentTimeMillis();
+      long timestamp = System.currentTimeMillis() - retainDuration;


nit: use consistent timestamp for all calculations

Suggested change

if ((lastKillTime + period) < System.currentTimeMillis()) {

lastKillTime = System.currentTimeMillis();

long timestamp = System.currentTimeMillis() - retainDuration;

long currentTimeMillis = System.currentTimeMillis();

if ((lastKillTime + period) < currentTimeMillis) {

lastKillTime = currentTimeMillis;

long timestamp = currentTimeMillis - retainDuration;

Additional question - I notice this pattern in a few other co-ordinator duties.

Are there any additional safeguards we need for a very large retainDuration? What happens if timestamp is calculated to be negative?

Added safeguard so that we never get calculated timestamp to be negative

Note that I intentionally didnt specific in the docs that retainDuration have to be less than current timestamp (although we do check against this condition to protect ourself from unexpected behavior), since this is a very unlikely scenario and I don't want to make the docs to be unnecessary verbose.

suneet-s · 2021-05-10T18:26:51Z

+      Set<String> allDatasourceWithActiveSupervisor = allSupervisor.values()
+                                                                   .stream()
+                                                                   // Terminated supervisor will have it's latest supervisorSpec as NoopSupervisorSpec
+                                                                   // (NoopSupervisorSpec is used as a tombstone marker)


This logic is very similar to SQLMetadataSupervisorManager#removeTerminatedSupervisorsOlderThan

Should that logic be moved out of the metadata store layer and pulled into the KillSupervisors class instead?

Should this logic be shared so that other callers can easily find the "active" supervisors?

I added some convenience methods in SQLMetadataSupervisorManager to getLatestTerminatedOnly and getLatestActiveOnly supervisors

suneet-s

LGTM after CI

[ERROR] Errors: 
[ERROR]   KillDatasourceMetadataTest.unnecessary Mockito stubbings » UnnecessaryStubbing

maytasm added 4 commits May 9, 2021 23:58

add auto clean up datasource metadata

1a830eb

add test

b096991

fix checkstyle

c2c4c32

add comments

92c37ab

asdf2014 added the Area - Metadata label May 10, 2021

fix error

2447830

suneet-s reviewed May 10, 2021

View reviewed changes

Comment thread server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java

suneet-s reviewed May 10, 2021

View reviewed changes

maytasm added 6 commits May 10, 2021 12:42

address comments

5032ecf

Address comments

3bda833

fix test

5ab2308

fix test

411bf44

fix typo

a3d57e3

add comment

f9c912c

suneet-s approved these changes May 11, 2021

View reviewed changes

maytasm added 2 commits May 10, 2021 22:25

fix test

50fd55b

fix test

118973d

maytasm merged commit 4326e69 into apache:master May 11, 2021

maytasm deleted the IMPLY-7006 branch May 11, 2021 08:22

techdocsmith mentioned this pull request May 12, 2021

add docs for high-churn datasource cleanup #11245

Merged

1 task

maytasm mentioned this pull request May 14, 2021

Auto cleanup of metadata tables to enable high frequency creation/deletion of data sources #11173

Closed

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

clintropolis mentioned this pull request Sep 3, 2021

[Draft] 0.22.0 Release Notes #11657

Closed

suneet-s mentioned this pull request Jan 21, 2022

Enable automatic metadata cleanup by default #12188

Merged

4 tasks

-    if ((lastKillTime + period) < System.currentTimeMillis()) {
-      lastKillTime = System.currentTimeMillis();
-      long timestamp = System.currentTimeMillis() - retainDuration;
+    long currentTimeMillis = System.currentTimeMillis();
+    if ((lastKillTime + period) < currentTimeMillis) {
+      lastKillTime = currentTimeMillis;
+      long timestamp = currentTimeMillis - retainDuration;

Conversation

maytasm commented May 10, 2021

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maytasm May 10, 2021 • 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!

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

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maytasm May 10, 2021 •

edited

Loading