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.

Sign up for GitHub

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

Jump to bottom

move Clickhouse HTTP handler to its own port. #5797

Merged
merged 6 commits into from
Jun 7, 2022
Merged

move Clickhouse HTTP handler to its own port. #5797

merged 6 commits into from
Jun 7, 2022

Conversation

youngsofun
Copy link
Member

@youngsofun youngsofun commented Jun 6, 2022 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

  1. default 8124, (clickhouse use 8123)
  2. :8000/clickhouse not remote in this pr.

Changelog

  • New Feature

Related Issues

Fixes #5728

@vercel
Copy link

vercel bot commented Jun 6, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) Jun 7, 2022 at 0:57AM (UTC)

@youngsofun youngsofun requested a review from ZhiHanZ June 6, 2022 07:42
@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Jun 6, 2022
Xuanwo
Xuanwo reviewed Jun 6, 2022
View reviewed changes
docker/databend-query-docker.toml
@@ -23,6 +23,10 @@ mysql_handler_port = 3307
clickhouse_handler_host = "0.0.0.0"
clickhouse_handler_port = 9001

# Databend Query ClickHouse Handler.
clickhouse_http_handler_host = "0.0.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

How about providing a clickhouse_http_handler_addr instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM, but not quite sure.

@youngsofun youngsofun force-pushed the clickhouse branch 2 times, most recently from 36d46e4 to 54170de Compare June 6, 2022 08:09
ZhiHanZ
ZhiHanZ approved these changes Jun 6, 2022
View reviewed changes
Copy link
Collaborator

@ZhiHanZ ZhiHanZ left a comment

Choose a reason for hiding this comment

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

nice work!

@youngsofun
Copy link
Member Author

failure tests seem not relevant:

Having 7 errors! 154 tests passed.                     0 tests skipped.
The failure tests:
    /workspace/tests/suites/0_stateless/02_function/02_0004_function_name_display.sql
    /workspace/tests/suites/0_stateless/03_dml/03_0004_select_order_by.sql
    /workspace/tests/suites/0_stateless/03_dml/03_0005_select_filter.sql
    /workspace/tests/suites/0_stateless/08_optimizer/08_0000_optimizer.sql
    /workspace/tests/suites/0_stateless/08_optimizer/08_0001_optimizer_statistics_exact.sql
    /workspace/tests/suites/0_stateless/09_fuse_engine/09_0009_remote_explain.sql
    /workspace/tests/suites/0_stateless/09_fuse_engine/09_0012_pushdown_limit.sql

BohuTANG
BohuTANG reviewed Jun 6, 2022
View reviewed changes
docs/doc/10-deploy/00-local.md Outdated
@@ -140,6 +140,10 @@ mysql_handler_port = 3307
clickhouse_handler_host = "127.0.0.1"
clickhouse_handler_port = 9001

# Query ClickHouse HTTP Handler.
clickhouse_http_handler_host = "127.0.0.1"
clickhouse_http_handler_port = 9001
Copy link
Member

Choose a reason for hiding this comment

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

Same as clickhouse_handler_port?

@BohuTANG
Copy link
Member

BohuTANG commented Jun 6, 2022

@mergify update

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2022

update

✅ Branch has been successfully updated

@BohuTANG
Copy link
Member

BohuTANG commented Jun 6, 2022

It seems that this PR break some tests.

@youngsofun
Copy link
Member Author

It seems that this PR break some tests.

my typo: dup ports in the config of servers for cluster test

@youngsofun
Copy link
Member Author

@BohuTANG fixed

@BohuTANG BohuTANG merged commit 31da2b5 into databendlabs:main Jun 7, 2022
@sundy-li
Copy link
Member

sundy-li commented Jun 8, 2022

@youngsofun Prefer to make clickhouse_http_handler_port default to 8123, some drivers will default to 8123 if it's not specified.

@youngsofun
Copy link
Member Author

@sundy-li user use this is likely to migrate from clickhouse, so he may have clickhouse running on his machine， in this situation， databend will fail to start. or maybe user wants to start both and do some compare?

is this why we use 3307 for MySQL? cc @BohuTANG

@youngsofun
Copy link
Member Author

@sundy-li use 3306 still can not help users who start a local cluster?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo left review comments

@BohuTANG BohuTANG BohuTANG left review comments

@ZhiHanZ ZhiHanZ ZhiHanZ approved these changes

Assignees
No one assigned
Labels
need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clickhouse http handler with its own port
6 participants
@youngsofun @BohuTANG @sundy-li @Xuanwo @ZhiHanZ @databend-bot