Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support configure System log table`s ttl in config.xml #17438

Merged
merged 5 commits into from
Dec 1, 2020

Conversation

spongedu
Copy link
Contributor

@spongedu spongedu commented Nov 26, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Allow specifying TTL to remove old entries from system log tables, using the <ttl> attribute in config.xml.

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Nov 26, 2020
@spongedu spongedu changed the title Support configure Ssystem log table's ttl in config.xml Support configure System log table`s ttl in config.xml Nov 26, 2020
@@ -589,6 +589,9 @@
toStartOfHour(event_time)
-->
<partition_by>toYYYYMM(event_date)</partition_by>
<!-- How many days data would be kept. It works only when this option is opened and is set greater than 0.
-->
<ttl>30</ttl>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name it <delete_after_days> or something, just <ttl> is too generic. Maybe we'll want to use it to accept generic TTL specification (or you can do it now if you wish, just paste it into table description).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name it <delete_after_days> or something, just <ttl> is too generic. Maybe we'll want to use it to accept generic TTL specification (or you can do it now if you wish, just paste it into table description).

ok, I think we can keep it as and make it accept generic TTL specification.

@@ -65,6 +65,9 @@ std::shared_ptr<TSystemLog> createSystemLog(
engine = "ENGINE = MergeTree";
if (!partition_by.empty())
engine += " PARTITION BY (" + partition_by + ")";
int ttl = config.getInt(config_prefix + ".ttl", 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a check to the above if, that if engine is specified manually, ttl must not be specified and must instead go into the engine specification. Like for <partition_by>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Will fix here.

@akuzm akuzm self-assigned this Nov 30, 2020
@spongedu
Copy link
Contributor Author

@akuzm Thanks for your review. I've made some updates according to our discussion. PTAL.

@akuzm
Copy link
Contributor

akuzm commented Dec 1, 2020

The test failures are not related.

This feature is very simple, but tricky to test, so I think we don't have to add a test for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants