tasks tables in metadata storage are not cleared #6592

seoeun25 · 2018-11-09T04:36:36Z

This PR mainly contains:

Remove the older tasks at metastore (druid_tasks) on tasklog cleanup schedule.
Not implement auditlog deletion since auditlogging is deprecated (Deprecate task audit logging #6368 )

core/src/main/java/org/apache/druid/metadata/MetadataStorageActionHandler.java

QiuMM · 2018-11-09T12:46:10Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/HeapMemoryTaskStorage.java

+                                  .map(entry -> entry.getKey())
+                                  .collect(Collectors.toList());
+
+      taskIds.stream().forEach(taskId -> tasks.remove(taskId));


Just taskIds.forEach(tasks::remove); would be ok.

QiuMM · 2018-11-09T12:52:19Z

core/src/main/java/org/apache/druid/metadata/MetadataStorageActionHandler.java

+   *
+   * @param createdTime datetime to check the {@code created_date} of tasks
+   */
+  void removeTasksBefore(DateTime createdTime);


maybe change to void removeTasksOlderThan(long timestamp) to keep consistent with TaskLogKiller#killOlderThan.

QiuMM · 2018-11-09T12:54:20Z

...ing-service/src/main/java/org/apache/druid/indexing/overlord/helpers/TaskLogAutoCleaner.java

@@ -68,6 +73,7 @@ public void run()
          {
            try {
              taskLogKiller.killOlderThan(System.currentTimeMillis() - config.getDurationToRetain());
+              taskStorage.removeTasksBefore(DateTimes.nowUtc().minus(config.getDurationToRetain()));


same, change to taskStorage.removeTasksOlderThan(System.currentTimeMillis() - config.getDurationToRetain())) for consistency.

QiuMM · 2018-11-09T12:58:48Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataStorageActionHandler.java

@@ -439,6 +439,26 @@ public Void withHandle(Handle handle)
    );
  }

+  @Override
+  public void removeTasksBefore(final DateTime createdTime)


please clean up the tasklogs table at the same time.

Have updated.

seoeun25 · 2018-11-13T05:47:02Z

It references the created_date at tasks table since tasklogs table have no created_date column.
It marks @deprecated to getSqlRemoveLogsOlderThan since tasklogs deprecated.

QiuMM · 2018-11-13T16:02:06Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskStorage.java

+   * Remove the tasks created order than the given createdTime.
+   *
+   * @param timestamp timestamp to check the {@code created_date} of tasks
+   */


I think the comment like below would be better:

/** * Remove the tasks created order than the given timestamp. * * @param timestamp timestamp in milliseconds */

Agree, please change "order" to "older" here as well.

QiuMM · 2018-11-13T16:12:28Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataStorageActionHandler.java

+  {
+    connector.retryWithHandle(
+        (HandleCallback<Void>) handle -> {
+          DateTime createdTime = DateTimes.utc(timestamp);


I do not like this variable name, it's a time point not a created time. Maybe DateTime dateTime = DateTimes.utc(timestamp);

QiuMM · 2018-11-13T16:13:46Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataStorageActionHandler.java

+                  entryTable
+              )
+          )
+                .bind("created_date", createdTime.toString())


Same, change created_date to another name.

QiuMM · 2018-11-13T16:18:59Z

It marks @deprecated to getSqlRemoveLogsOlderThan since tasklogs deprecated.

@seoeun25 Auditlog has been deprecated, not tasklogs.

surekhasaharan · 2018-11-13T19:22:28Z

core/src/main/java/org/apache/druid/metadata/MetadataStorageActionHandler.java

@@ -130,6 +130,13 @@ void insert(
   */
  void removeLock(long lockId);

+  /**
+   * Remove the tasks created order than the given createdTime.


nit: change "order" or "older". Or may be better way is "Remove the tasks with timestamp older than given timestamp

surekhasaharan · 2018-11-13T19:25:03Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskStorage.java

+   * Remove the tasks created order than the given createdTime.
+   *
+   * @param timestamp timestamp to check the {@code created_date} of tasks
+   */


Agree, please change "order" to "older" here as well.

gianm · 2018-11-13T21:45:44Z

It looks like this is adding new behavior (deleting old tasks) to a pre-existing config (druid.indexer.logs.kill.durationToRetain). Is that right? What's the default for these? I'm just thinking about whether people will be surprised by the new behavior.

At any rate, I'll add a "release notes" label so we can call out what would change, if anything.

This patch should also include a doc update.

seoeun25 · 2018-11-14T06:46:45Z

This commit contains:

change the variable name and comment.
update the doc to druid.indexer.logs.kill.enabled.

jihoonson

@seoeun25 thanks for the contribution! I left a few trivial comments. Also, would you please add more detailed docs which describe that druid.indexer.logs.kill.enabled will update the tasks and tasklogs tables as well?

jihoonson · 2018-11-19T18:57:38Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataStorageActionHandler.java

+    connector.retryWithHandle(
+        (HandleCallback<Void>) handle -> {
+          DateTime dateTime = DateTimes.utc(timestamp);
+          log.info("Deleting tasks older than [%s] and inactive at metastore.", dateTime.toString());


I think it's better to add a log in the caller than here because this will be printed per retry.

jihoonson · 2018-11-19T20:21:28Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataStorageActionHandler.java

+          DateTime dateTime = DateTimes.utc(timestamp);
+          log.info("Deleting tasks older than [%s] and inactive at metastore.", dateTime.toString());
+          handle.createStatement(
+              StringUtils.format(


I guess this is not needed?

jihoonson · 2018-11-19T20:28:06Z

@seoeun25 Auditlog has been deprecated, not tasklogs.

@QiuMM, it's quite confusing, but @seoeun25 is correct. tasklogs is the table for task audit logging which is deprecated now. The audit table is for auditing rule changes.

QiuMM · 2018-11-20T02:50:57Z

@jihoonson @seoeun25 oops, I had not checked it carefully, sorry.

seoeun25 · 2018-11-20T06:43:53Z

@QiuMM @surekhasaharan @jihoonson @gianm Thanks for the review. Addressed comments and updated doc more detailed.

jihoonson

@QiuMM no worries!

@seoeun25 thanks for the quick fix! I left one more comment.

jihoonson · 2018-11-20T22:11:27Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataStorageActionHandler.java

+  public void removeTasksOlderThan(final long timestamp)
+  {
+    DateTime dateTime = DateTimes.utc(timestamp);
+    log.info("Deleting tasks older than [%s] and inactive at metastore.", dateTime.toString());


Sorry, I was not clear. I think it's better to remove log from this class (which shouldn't be part of this PR) because it's not very useful. Logging should give us detailed context about what's happening, but a simple I will delete some tasks from metadata message is not much useful I think. Rather, it looks good to me to print logs in the caller of this method, like in TaskAutoCleaner.

I agree. I think it's better to remove log.

surekhasaharan · 2018-11-20T22:41:06Z

docs/content/configuration/index.md

 Caution: Automatic log file deletion typically works based on log file modification timestamp on the backing store, so large clock skews between druid nodes and backing store nodes might result in un-intended behavior.  

 |Property|Description|Default|
 |--------|-----------|-------|
-|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. |false|
+|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. If set to true, overlord will submit tasks periodically based on `delay` specified. These kill tasks will delete task logs from log directory and tasks and tasklogs table in metadata storage except for the last `durationToRetain` period. |false|


How about "If set to true, overlord will submit kill tasks periodically based on delay specified, which will delete task logs from the log directory as well as tasks and tasklogs table in metadata storage except for tasks created in the last durationToRetain period. "

Thanks for your comment !

surekhasaharan · 2018-11-20T22:44:39Z

docs/content/configuration/index.md

 Caution: Automatic log file deletion typically works based on log file modification timestamp on the backing store, so large clock skews between druid nodes and backing store nodes might result in un-intended behavior.  

 |Property|Description|Default|
 |--------|-----------|-------|
-|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. |false|
+|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. If set to true, overlord will submit tasks periodically based on `delay` specified. These kill tasks will delete task logs from log directory and tasks and tasklogs table in metadata storage except for the last `durationToRetain` period. |false|
 |`druid.indexer.logs.kill.durationToRetain`| Required if kill is enabled. In milliseconds, task logs to be retained created in last x milliseconds. |None|


If this behavior is also getting changed, please add doc here as well, something like "task logs and task-related metadata storage tables to be retained created in last x milliseconds"

seoeun25 · 2018-11-21T01:52:02Z

@jihoonson @surekhasaharan Thanks for the review. remove redundant log and update doc more detailed.

QiuMM · 2018-11-21T03:33:04Z

core/src/main/java/org/apache/druid/metadata/MetadataStorageActionHandler.java

@@ -130,6 +130,13 @@ void insert(
   */
  void removeLock(long lockId);

+  /**
+   * Remove the tasks with timestamp older than given timestamp.
+   * 


Remove the tasks created older than the given timestamp. would be better.

QiuMM · 2018-11-21T03:35:55Z

docs/content/configuration/index.md

@@ -525,13 +525,13 @@ If you are running the indexing service in remote mode, the task logs must be st
 |--------|-----------|-------|
 |`druid.indexer.logs.type`|Choices:noop, s3, azure, google, hdfs, file. Where to store task logs|file|

-You can also configure the Overlord to automatically retain the task logs only for last x milliseconds by configuring following additional properties.
+You can also configure the Overlord to automatically retain the task logs in log directory and task-related metadata storage tables only for last x milliseconds by configuring following additional properties.
 Caution: Automatic log file deletion typically works based on log file modification timestamp on the backing store, so large clock skews between druid nodes and backing store nodes might result in un-intended behavior.  


...in log directory and entries in task-related metadata storage tables... would be better.

QiuMM · 2018-11-21T03:47:43Z

docs/content/configuration/index.md

 Caution: Automatic log file deletion typically works based on log file modification timestamp on the backing store, so large clock skews between druid nodes and backing store nodes might result in un-intended behavior.  

 |Property|Description|Default|
 |--------|-----------|-------|
-|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. |false|
-|`druid.indexer.logs.kill.durationToRetain`| Required if kill is enabled. In milliseconds, task logs to be retained created in last x milliseconds. |None|
+|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. If set to true, overlord will submit kill tasks periodically based on `delay` specified, which will delete task logs from the log directory as well as tasks and tasklogs table in metadata storage except for tasks created in the last `durationToRetain` period. |false|


I think

....based on `druid.indexer.logs.kill.delay` specified, which will delete task logs from the log directory as well as tasks and tasklogs table entries in metadata storage except for tasks created in the last `druid.indexer.logs.kill.durationToRetain` period.

would be better.

QiuMM · 2018-11-21T03:49:30Z

docs/content/configuration/index.md

-|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. |false|
-|`druid.indexer.logs.kill.durationToRetain`| Required if kill is enabled. In milliseconds, task logs to be retained created in last x milliseconds. |None|
+|`druid.indexer.logs.kill.enabled`|Boolean value for whether to enable deletion of old task logs. If set to true, overlord will submit kill tasks periodically based on `delay` specified, which will delete task logs from the log directory as well as tasks and tasklogs table in metadata storage except for tasks created in the last `durationToRetain` period. |false|
+|`druid.indexer.logs.kill.durationToRetain`| Required if kill is enabled. In milliseconds, task logs and task-related metadata storage tables to be retained created in last x milliseconds. |None|


Same, ...task logs and entries in task-related....

jihoonson

LGTM other than @QiuMM's comments.

seoeun25 · 2018-11-22T00:12:44Z

@QiuMM Thanks for the comments! updated doc.

…he#6592) * tasks tables in metadata storage are not cleared * address comments. remove tasklogs and revert obsolete changes * address comments. change comment and update doc. * address comments. update doc more detailed * address comments. remove redundant log and update doc more detailed. * address comments. update document

…age are not cleared 6592) * tasks tables in metadata storage are not cleared * address comments. remove tasklogs and revert obsolete changes * address comments. change comment and update doc. * address comments. update doc more detailed * address comments. remove redundant log and update doc more detailed. * address comments. update document

tasks tables in metadata storage are not cleared

c0caaff

QiuMM reviewed Nov 9, 2018

View reviewed changes

core/src/main/java/org/apache/druid/metadata/MetadataStorageActionHandler.java Outdated Show resolved Hide resolved

QiuMM reviewed Nov 9, 2018

View reviewed changes

address comments. remove tasklogs and revert obsolete changes

eb90baa

QiuMM reviewed Nov 13, 2018

View reviewed changes

surekhasaharan reviewed Nov 13, 2018

View reviewed changes

gianm assigned jihoonson Nov 13, 2018

gianm added the Release Notes label Nov 13, 2018

gianm added this to the 0.13.1 milestone Nov 13, 2018

address comments. change comment and update doc.

acbc4c8

jihoonson reviewed Nov 19, 2018

View reviewed changes

address comments. update doc more detailed

c31b4fe

jihoonson reviewed Nov 20, 2018

View reviewed changes

surekhasaharan reviewed Nov 20, 2018

View reviewed changes

address comments. remove redundant log and update doc more detailed.

f428cba

QiuMM reviewed Nov 21, 2018

View reviewed changes

address comments. update document

b60e06d

jihoonson approved these changes Nov 21, 2018

View reviewed changes

QiuMM approved these changes Nov 22, 2018

View reviewed changes

QiuMM merged commit 22a5bf9 into apache:master Nov 22, 2018

seoeun25 deleted the apache-6579 branch November 22, 2018 05:14

jon-wei mentioned this pull request Feb 22, 2019

0.14.0-incubating release notes #7126

Closed

xvrl mentioned this pull request Feb 3, 2022

Automatic cleanup druid_tasks table of metastore #5994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tasks tables in metadata storage are not cleared #6592

tasks tables in metadata storage are not cleared #6592

seoeun25 commented Nov 9, 2018

QiuMM Nov 9, 2018

QiuMM Nov 9, 2018

QiuMM Nov 9, 2018

QiuMM Nov 9, 2018

seoeun25 Nov 13, 2018

seoeun25 commented Nov 13, 2018 •

edited

Loading

QiuMM Nov 13, 2018

surekhasaharan Nov 13, 2018

QiuMM Nov 13, 2018

QiuMM Nov 13, 2018

QiuMM commented Nov 13, 2018

surekhasaharan Nov 13, 2018

surekhasaharan Nov 13, 2018

gianm commented Nov 13, 2018

seoeun25 commented Nov 14, 2018

jihoonson left a comment

jihoonson Nov 19, 2018

jihoonson Nov 19, 2018

jihoonson commented Nov 19, 2018

QiuMM commented Nov 20, 2018

seoeun25 commented Nov 20, 2018

jihoonson left a comment

jihoonson Nov 20, 2018

seoeun25 Nov 21, 2018

surekhasaharan Nov 20, 2018

seoeun25 Nov 21, 2018

surekhasaharan Nov 20, 2018

seoeun25 commented Nov 21, 2018

QiuMM Nov 21, 2018

QiuMM Nov 21, 2018

QiuMM Nov 21, 2018

QiuMM Nov 21, 2018

jihoonson left a comment

seoeun25 commented Nov 22, 2018

tasks tables in metadata storage are not cleared #6592

tasks tables in metadata storage are not cleared #6592

Conversation

seoeun25 commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seoeun25 commented Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiuMM commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm commented Nov 13, 2018

seoeun25 commented Nov 14, 2018

jihoonson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson commented Nov 19, 2018

QiuMM commented Nov 20, 2018

seoeun25 commented Nov 20, 2018

jihoonson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seoeun25 commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

seoeun25 commented Nov 22, 2018

seoeun25 commented Nov 13, 2018 •

edited

Loading