You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/04/27 04:36:36 UTC
[GitHub] [druid] maytasm opened a new pull request #11164: Add feature to automatically remove rules based on retention period
maytasm opened a new pull request #11164:
URL: https://github.com/apache/druid/pull/11164
Add feature to automatically remove rules based on retention period
### Description
We currently already have tasklog auto cleanup (https://github.com/apache/druid/pull/3677) and audit logs auto cleanup (https://github.com/apache/druid/pull/11084). This PR adds a similar auto cleanup based on duration (time to retained) but for the rules table to auto clean up rules of inactive datasource (datasource with no used and unused segments)
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:
- [x] 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.)
- [x] added documentation for new or modified features or behaviors.
- [x] 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)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] 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.
- [x] 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.
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
[GitHub] [druid] jihoonson commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r623379648
##########
File path: server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java
##########
@@ -421,8 +422,35 @@ public Void inTransaction(Handle handle, TransactionStatus transactionStatus) th
return true;
}
+ @Override
+ public int removeRulesOlderThan(long timestamp)
+ {
+ DateTime dateTime = DateTimes.utc(timestamp);
+ synchronized (lock) {
+ return dbi.withHandle(
+ handle -> {
+ Update sql = handle.createStatement(
+ StringUtils.format(
+ "DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time",
Review comment:
It makes sense đź‘Ť
--
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.
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
[GitHub] [druid] maytasm commented on pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #11164:
URL: https://github.com/apache/druid/pull/11164#issuecomment-831459836
> Overall LGTM
>
> Is it possible for a user to create a segment with a custom version? What would happen in these cases? Do we need guardrails to prevent users from enabling this feature if there are segments that are not in the UTC format?
It is not possible for a user to specify a custom version. The only way possible would be for a user to connect to the metadata store directly and issue SQL to modify the version field. I don't think we need to consider this as that is not supported for regular operation.
--
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.
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
[GitHub] [druid] jihoonson commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r622661391
##########
File path: server/src/main/java/org/apache/druid/metadata/MetadataRuleManager.java
##########
@@ -42,4 +42,12 @@
List<Rule> getRulesWithDefault(String dataSource);
boolean overrideRule(String dataSource, List<Rule> rulesConfig, AuditInfo auditInfo);
+
+ /**
+ * Remove rules for non-existence datasource (datasource with no segment) created older than the given timestamp.
+ *
+ * @param timestamp timestamp in milliseconds
+ * @return number of rules removed
+ */
+ int removeRulesOlderThan(long timestamp);
Review comment:
Maybe `removeRulesForEmptyDatasourcesOlderThan` is better
##########
File path: server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java
##########
@@ -421,8 +422,35 @@ public Void inTransaction(Handle handle, TransactionStatus transactionStatus) th
return true;
}
+ @Override
+ public int removeRulesOlderThan(long timestamp)
+ {
+ DateTime dateTime = DateTimes.utc(timestamp);
+ synchronized (lock) {
+ return dbi.withHandle(
+ handle -> {
+ Update sql = handle.createStatement(
+ StringUtils.format(
+ "DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time",
Review comment:
Also, this query could be expensive when the segments table is large. I think maybe it's better to implement the join in this class. For example, this method can iterate all datasources in the rules table and check whether there is any entry in the segments table for each datasource. Then, you can use a much cheaper query for the existence check such as `SELECT datasource FROM segments where datasource = ${rule_datasource} LIMIT 1`. Maybe this approach needs to add a new index on `datasource` for the segments table.
##########
File path: server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java
##########
@@ -421,8 +422,35 @@ public Void inTransaction(Handle handle, TransactionStatus transactionStatus) th
return true;
}
+ @Override
+ public int removeRulesOlderThan(long timestamp)
+ {
+ DateTime dateTime = DateTimes.utc(timestamp);
+ synchronized (lock) {
+ return dbi.withHandle(
+ handle -> {
+ Update sql = handle.createStatement(
+ StringUtils.format(
+ "DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time",
Review comment:
After this change, the `version` must be the timestamp of when the rule entry was added. It means [this code](https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java#L381) must remain the same as it is unless we change this query together. Please add comments on both codes so that we won't forget 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.
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
[GitHub] [druid] suneet-s commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r625156598
##########
File path: docs/configuration/index.md
##########
@@ -750,6 +750,9 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|`druid.coordinator.kill.audit.on`| Boolean value for whether to enable automatic deletion of audit logs. If set to true, Coordinator will periodically remove audit logs from the audit table entries in metadata storage.| No | False|
|`druid.coordinator.kill.audit.period`| How often to do automatic deletion of audit logs in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Value must be greater than `druid.coordinator.period.metadataStoreManagementPeriod`. Only applies if `druid.coordinator.kill.audit.on` is set to True.| No| `P1D`|
|`druid.coordinator.kill.audit.durationToRetain`| Duration of audit logs to be retained from created time in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Only applies if `druid.coordinator.kill.audit.on` is set to True.| Yes if `druid.coordinator.kill.audit.on` is set to True| None|
+|`druid.coordinator.kill.rule.on`| Boolean value for whether to enable automatic deletion of rules. If set to true, Coordinator will periodically remove rules of inactive datasource (datasource with no used and unused segments) from the rule table in metadata storage.| No | False|
+|`druid.coordinator.kill.rule.period`| How often to do automatic deletion of rules in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Value must be greater than `druid.coordinator.period.metadataStoreManagementPeriod`. Only applies if `druid.coordinator.kill.rule.on` is set to True.| No| `P1D`|
+|`druid.coordinator.kill.rule.durationToRetain`| Duration of rules to be retained from created time in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Only applies if `druid.coordinator.kill.rule.on` is set to True.| Yes if `druid.coordinator.kill.rule.on` is set to True| None|
Review comment:
We should update this to say that this will only work as expected if the version is a timestamp in utc
--
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.
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
[GitHub] [druid] maytasm commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r622703819
##########
File path: server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java
##########
@@ -421,8 +422,35 @@ public Void inTransaction(Handle handle, TransactionStatus transactionStatus) th
return true;
}
+ @Override
+ public int removeRulesOlderThan(long timestamp)
+ {
+ DateTime dateTime = DateTimes.utc(timestamp);
+ synchronized (lock) {
+ return dbi.withHandle(
+ handle -> {
+ Update sql = handle.createStatement(
+ StringUtils.format(
+ "DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time",
Review comment:
Regarding the query `"DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time"`, I think since it’s run very infrequent (by default once a day) and the inner query on segment table is a READ (no locking), it should be ok. Also, adding an index on datasource column in the segment table (for the alternative approach suggested) puts additional overhead on the database operation which is probably not worth it for a query that is run once a day. I think it is fine to keep it as is. I also added a comment in the code regarding this.
--
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.
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
[GitHub] [druid] maytasm commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r622702813
##########
File path: server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java
##########
@@ -421,8 +422,35 @@ public Void inTransaction(Handle handle, TransactionStatus transactionStatus) th
return true;
}
+ @Override
+ public int removeRulesOlderThan(long timestamp)
+ {
+ DateTime dateTime = DateTimes.utc(timestamp);
+ synchronized (lock) {
+ return dbi.withHandle(
+ handle -> {
+ Update sql = handle.createStatement(
+ StringUtils.format(
+ "DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time",
Review comment:
Added comment on `version` as suggested.
--
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.
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
[GitHub] [druid] maytasm commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r622703819
##########
File path: server/src/main/java/org/apache/druid/metadata/SQLMetadataRuleManager.java
##########
@@ -421,8 +422,35 @@ public Void inTransaction(Handle handle, TransactionStatus transactionStatus) th
return true;
}
+ @Override
+ public int removeRulesOlderThan(long timestamp)
+ {
+ DateTime dateTime = DateTimes.utc(timestamp);
+ synchronized (lock) {
+ return dbi.withHandle(
+ handle -> {
+ Update sql = handle.createStatement(
+ StringUtils.format(
+ "DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time",
Review comment:
Regarding the query `"DELETE FROM %1$s WHERE datasource NOT IN (SELECT DISTINCT datasource from %2$s) and datasource!=:default_rule and version < :date_time"`, I think since it’s run very infrequent (by default once a day) and the inner query on segment table is a READ (no locking), it should be ok. Also, adding an index on datasource column in the segment table (for the alternative approach suggested) puts additional overhead on the database operation which is probably not worth it for a query that is run once a day.
--
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.
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
[GitHub] [druid] maytasm commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r622701627
##########
File path: server/src/main/java/org/apache/druid/metadata/MetadataRuleManager.java
##########
@@ -42,4 +42,12 @@
List<Rule> getRulesWithDefault(String dataSource);
boolean overrideRule(String dataSource, List<Rule> rulesConfig, AuditInfo auditInfo);
+
+ /**
+ * Remove rules for non-existence datasource (datasource with no segment) created older than the given timestamp.
+ *
+ * @param timestamp timestamp in milliseconds
+ * @return number of rules removed
+ */
+ int removeRulesOlderThan(long timestamp);
Review comment:
Done
--
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.
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
[GitHub] [druid] loquisgon commented on pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
loquisgon commented on pull request #11164:
URL: https://github.com/apache/druid/pull/11164#issuecomment-828059591
LGTM
--
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.
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
[GitHub] [druid] suneet-s commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r625156598
##########
File path: docs/configuration/index.md
##########
@@ -750,6 +750,9 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|`druid.coordinator.kill.audit.on`| Boolean value for whether to enable automatic deletion of audit logs. If set to true, Coordinator will periodically remove audit logs from the audit table entries in metadata storage.| No | False|
|`druid.coordinator.kill.audit.period`| How often to do automatic deletion of audit logs in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Value must be greater than `druid.coordinator.period.metadataStoreManagementPeriod`. Only applies if `druid.coordinator.kill.audit.on` is set to True.| No| `P1D`|
|`druid.coordinator.kill.audit.durationToRetain`| Duration of audit logs to be retained from created time in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Only applies if `druid.coordinator.kill.audit.on` is set to True.| Yes if `druid.coordinator.kill.audit.on` is set to True| None|
+|`druid.coordinator.kill.rule.on`| Boolean value for whether to enable automatic deletion of rules. If set to true, Coordinator will periodically remove rules of inactive datasource (datasource with no used and unused segments) from the rule table in metadata storage.| No | False|
+|`druid.coordinator.kill.rule.period`| How often to do automatic deletion of rules in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Value must be greater than `druid.coordinator.period.metadataStoreManagementPeriod`. Only applies if `druid.coordinator.kill.rule.on` is set to True.| No| `P1D`|
+|`druid.coordinator.kill.rule.durationToRetain`| Duration of rules to be retained from created time in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Only applies if `druid.coordinator.kill.rule.on` is set to True.| Yes if `druid.coordinator.kill.rule.on` is set to True| None|
Review comment:
We should update this to say that this will only work as expected if the version is a timestamp
--
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.
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
[GitHub] [druid] maytasm merged pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #11164:
URL: https://github.com/apache/druid/pull/11164
--
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.
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
[GitHub] [druid] maytasm commented on a change in pull request #11164: Add feature to automatically remove rules based on retention period
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11164:
URL: https://github.com/apache/druid/pull/11164#discussion_r625292642
##########
File path: docs/configuration/index.md
##########
@@ -750,6 +750,9 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|`druid.coordinator.kill.audit.on`| Boolean value for whether to enable automatic deletion of audit logs. If set to true, Coordinator will periodically remove audit logs from the audit table entries in metadata storage.| No | False|
|`druid.coordinator.kill.audit.period`| How often to do automatic deletion of audit logs in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Value must be greater than `druid.coordinator.period.metadataStoreManagementPeriod`. Only applies if `druid.coordinator.kill.audit.on` is set to True.| No| `P1D`|
|`druid.coordinator.kill.audit.durationToRetain`| Duration of audit logs to be retained from created time in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Only applies if `druid.coordinator.kill.audit.on` is set to True.| Yes if `druid.coordinator.kill.audit.on` is set to True| None|
+|`druid.coordinator.kill.rule.on`| Boolean value for whether to enable automatic deletion of rules. If set to true, Coordinator will periodically remove rules of inactive datasource (datasource with no used and unused segments) from the rule table in metadata storage.| No | False|
+|`druid.coordinator.kill.rule.period`| How often to do automatic deletion of rules in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Value must be greater than `druid.coordinator.period.metadataStoreManagementPeriod`. Only applies if `druid.coordinator.kill.rule.on` is set to True.| No| `P1D`|
+|`druid.coordinator.kill.rule.durationToRetain`| Duration of rules to be retained from created time in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) duration format. Only applies if `druid.coordinator.kill.rule.on` is set to True.| Yes if `druid.coordinator.kill.rule.on` is set to True| None|
Review comment:
Will add in a followup PR
--
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.
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