Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

fix shards #67

Merged
merged 6 commits into from
Mar 23, 2022
Merged

fix shards #67

merged 6 commits into from
Mar 23, 2022

Conversation

hruljov
Copy link
Contributor

@hruljov hruljov commented Dec 1, 2021

No description provided.

@AntonFriberg
Copy link
Contributor

@AlexeySetevoi Could we perhaps retrigger the tests on this one? They seem related to #71

@AlexeySetevoi
Copy link
Owner

Yes, the failed tests in this PR are associated with #71.
Since #71 changed the test declaration, this PR needs to be rebased to master.
Manual restart in this current PR state - will just fall again.

@AntonFriberg
Copy link
Contributor

@AlexeySetevoi This needs approval from you in order to run CI tests

README.md Outdated
@@ -26,11 +26,20 @@ F: You can add listen ips on top of defaults:
clickhouse_listen_host_custom:
- "192.168.0.1"
```
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a misstake due to merge conflict

README.md Outdated
F: Or you can specify ips directly e.g. to listen on all ipv4 and ipv6 addresses:
```yaml
clickhouse_listen_host:
- "::"
```
>>>>>>> 7cb5558ba9f17238a4f33d2ba3b0f5e7c6ed7003
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks weird

F: you can manage ttl query_log:
```yaml
clickhouse_query_log_ttl: 'event_date + INTERVAL 7 DELETE'
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not have anything to do with the fixing of shards? It should perhaps be better suited in a separate PR?

README.md Outdated
@@ -244,6 +253,7 @@ Including an example of how to use your role (for instance, with variables passe
quota: "default",
dbs: [ testu1,testu2,testu3 ] ,
comment: "classic user with multi dbs and multi-custom network allow password"}
clickhouse_query_log_ttl: 'event_date + INTERVAL 30 DELETE'
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate PR?

@@ -264,6 +264,9 @@
toStartOfHour(event_time)
-->
<partition_by>toYYYYMM(event_date)</partition_by>
{% if clickhouse_query_log_ttl is defined %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate PR?

{% endfor %}
</remote_servers>
</yandex>
</yandex>
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is probaly also unintentional. You are missing final newline character

Copy link
Contributor

@AntonFriberg AntonFriberg left a comment

Choose a reason for hiding this comment

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

Saw some issues with this that I would guess Alexey would want fixed before he can merge this. Let me know if you have any questions.

@hruljov
Copy link
Contributor Author

hruljov commented Mar 17, 2022

Thank you, I'm very inattentive, I fix it

@AntonFriberg
Copy link
Contributor

@AlexeySetevoi Could you start the CI tests on this one?

@hruljov
Copy link
Contributor Author

hruljov commented Mar 23, 2022

@AlexeySetevoi merge?

@AlexeySetevoi AlexeySetevoi merged commit 41f4126 into AlexeySetevoi:master Mar 23, 2022
AntonFriberg pushed a commit to AntonFriberg/ansible-clickhouse that referenced this pull request Mar 25, 2022
* shard

* fix

* add clickhouse_query_log_ttl

* fix readme

Co-authored-by: Maksim <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants