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

Update MIGRATIONS.md #3975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update MIGRATIONS.md #3975

wants to merge 1 commit into from

Conversation

dbanda
Copy link
Contributor

@dbanda dbanda commented Apr 4, 2023

Add description of the correct query node value

Add description of the correct query node value
@dbanda dbanda marked this pull request as ready for review April 4, 2023 19:36
@dbanda dbanda requested a review from a team as a code owner April 4, 2023 19:36
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (63ad356) 90.21% compared to head (276402e) 90.21%.

❗ Current head 276402e differs from pull request most recent head 33e4c89. Consider uploading reports for the commit 33e4c89 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3975   +/-   ##
=======================================
  Coverage   90.21%   90.21%           
=======================================
  Files         750      750           
  Lines       36397    36397           
  Branches      188      188           
=======================================
  Hits        32835    32835           
  Misses       3533     3533           
  Partials       29       29           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -48,6 +48,8 @@ For each cluster, `single_node` can be set to either True or False. If False, `c

For a single node cluster, do not change the default configuration.

For multi node clusters (`single_node=False`), the `host` and `port` of the cluster mappings must point to a node that will have the distributed tables. Typically, this is a node that belongs to the `distributed_cluster_name` cluster in clickhouse. This node is used as the query node. In our production environment, the `host` value is a loadbalancer proxy over the nodes is the the `distributed_cluster_name`
Copy link
Member

Choose a reason for hiding this comment

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

the host value is a loadbalancer proxy over the nodes is the the distributed_cluster_name

I'm not sure what this means.

Copy link
Member

Choose a reason for hiding this comment

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

Typically, this is a node that belongs to the distributed_cluster_name cluster in clickhouse.

I question whether this is actually typical. At least it seems not at Sentry. Afaik we often have no distributed cluster at all (local, test, small self hosted envs) or multiple distributed tables with a loadbalancer in front, but not only one node in the distributed cluster like you described

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For cases where there's no distributed cluster, single_node = True and everything is in one host. What I was trying to communicate is that the query node has to have access to the dist tables in the non single node case because we don't make that clear anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Does this make anything clearer?

For multi node clusters (single_node=False), the host and port of the cluster mappings must point to a node that will have the distributed tables. If distributed tables are separated onto their own node (query node) and you have more than one of these query nodes, you should have a ClickHouse cluster defined for those nodes. That cluster name is used for the distributed_cluster_name. The host value is this scenario could either be the query node directly or a load balancer proxy in front of the query nodes.

@getsantry getsantry bot added the Stale label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants