Rewrite clickhouse-test to use python clickhouse_driver #29856

azat · 2021-10-07T18:10:23Z

Pros:

Using native protocol over executing binaries is always better
clickhouse-client in debug build takes almost a second to execute simple SELECT 1
and clickhouse-test requires ~5 queries at start (determine some
flags, zk, alive, create database)

Notes:

FORMAT Vertical had been replaced with printing of pandas.DataFrame

And after this patch tiny tests work with the speed of the test, and
does not requires +-5 seconds of bootstrapping.

Changelog category (leave one):

Not for changelog (changelog entry is not required)

azat · 2021-10-07T18:37:33Z

Can some one add force test label please? (it requires updating of fast test image which is not built per-PR AFAIK)

nikitamikhaylov · 2021-10-07T18:43:46Z

@Mergifyio update

mergify · 2021-10-07T18:43:53Z

Command update: success

Branch already up to date

alexey-milovidov · 2021-10-07T19:35:46Z

But it does not test clickhouse-client.
It's worth doing for debug and sanitizers builds.

azat · 2021-10-07T20:35:01Z

@alexey-milovidov this PR converts only service use of clickhouse-client i.e. to:

run show processlist
to health check (simple select 1 or select {test_name})
create database for tests

Every .sql test that was started using clickhouse-client will still use it.

…rately)

Pros: - Using native protocol over executing binaries is always better - `clickhouse-client` in debug build takes almost a second to execute simple `SELECT 1` and `clickhouse-test` requires ~5 queries at start (determine some flags, zk, alive, create database) Notes: - `FORMAT Vertical` had been replaced with printing of `pandas.DataFrame` And after this patch tiny tests work with the speed of the test, and does not requires +-5 seconds of bootstrapping.

… to threads)

amosbird · 2021-10-08T03:13:10Z

How about adding an option to run tests with clickhouse_driver as this is only useful for developers.

alexey-milovidov · 2021-10-08T10:44:40Z

this PR converts only service use of clickhouse-client

Ok.

tests/clickhouse-test

azat · 2021-10-08T10:53:15Z

How about adding an option to run tests with clickhouse_driver as this is only useful for developers.

This will too strange. Then, I would prefer to just leave it as-is (i.e. close this PR).
@amosbird Why do you think that separate code path is required for this?

I've converted it to python driver to make it faster since executing binaries (even from release build) will be always slower then simply connect+send+recv (i.e. native protocol).
My use case, once I've added some test I want to check it, and I'm waiting for clickhouse-test to do some checks that takes +-5 seconds with debug build.

alexey-milovidov · 2021-10-08T11:08:10Z

I've converted it to python driver to make it faster since executing binaries (even from release build) will be always slower then simply connect+send+recv (i.e. native protocol).

Ok. I think it's worth doing.
But let's also improve the startup time of clickhouse-client (in separate PR).

for i in {1..10000}; do clickhouse-local --query "SELECT 1"; done

it's spending time in initializing DateLUT - for what?

amosbird · 2021-10-08T11:12:04Z

Why do you think that separate code path is required for this?

I missed this reply "this PR converts only service use of clickhouse-client" and thought it was to replace clickhouse-client for .sql tests)

https://clickhouse-test-reports.s3.yandex.net/29856/e2d6698244d43979b3fe2478dfdcd8dc3a91a0fd/stress_test_(address).html#fail1

https://clickhouse-test-reports.s3.yandex.net/29856/e2d6698244d43979b3fe2478dfdcd8dc3a91a0fd/style_check/test_run.txt.out.log

amosbird · 2021-10-09T02:20:18Z

Btw, why don't we just use HTTP for this purpose instead?

azat · 2021-10-09T09:07:07Z

Btw, why don't we just use HTTP for this purpose instead?

I don't see any benefits in using HTTP instead of TCP.
Plus you need to parse it (since we don't only need to execute queries, and check return values, but also there are things like show processlist)

azat · 2021-10-09T09:09:21Z

BTW looks like everything related had been passed!

Fast test — Cannot install or start ClickHouse

Requires image update (@alesapin I guess it will be done automatically after merge?).

azat · 2021-10-09T09:33:47Z

Btw, why don't we just use HTTP for this purpose instead?

Ok, I haven't understand that clear enough.

@amosbird is right, HTTP interface can be also used instead of clickhouse-driver (python).

Cons:

~~one more extra dependency~~

Pros:

has better error handling (i.e. defines lots of exceptions for various kind of errors, and also provides lots of predefined error codes)
~~provide better API for reading (streaming and so on), but it is not used right now~~
it is already used for performance tests, so the cost of adding this "extra dependency" should be minor (although I guess not every developer runs performance tests by themselves, but they do run stateless tests for sure)

So maybe someone else has some opinion on this?

alexey-milovidov · 2021-10-09T12:03:14Z

I don't have stong opinion on that - let's use what will be more convenient.

azat · 2021-10-09T12:52:11Z

Integration tests (asan) — fail: 3, passed: 1585, flaky: 4
Integration tests (release) — fail: 3, passed: 1588, flaky: 3
Integration tests (thread) — fail: 5, passed: 1583, flaky: 5

test_backup_restore - Fix flaky integration tests (test_backup_restore/test_input_format_parallel_parsing_memory_tracking) #29919
test_input_format_parallel_parsing_memory_tracking/test.py::test_memory_tracking_total - Fix flaky integration tests (test_backup_restore/test_input_format_parallel_parsing_memory_tracking) #29919
test_cluster_copier - Fix filtering by tuple (some conditions was lost during analyzing) #29956
test_dictionaries_dependency_xml/test.py::test_get_data - Fix lock-order-inversion between periodic dictionary reload and config reload #29928
Stress test (thread) — Sanitizer assert (in stderr.log) - Fix lock-order-inversion between DROP TABLE for DatabaseMemory and LiveView #29929

Also note, that this should make integration tests green.

azat · 2021-10-09T22:20:11Z

Some numbers:

fasttest: before 11min/14min, after 9min/12min -- no benefit
stateless debug: no benefit

SELECT DISTINCT
    check_duration_ms,
    if(report_url LIKE '%268c155b7d4b15524219ea48cf228bf140cf9cdd%', 'python-requests', '') AS mr
FROM checks
WHERE (check_start_time > today()) AND (pull_request_number = 0) AND (report_url LIKE '%stateless_tests_(debug)%')
ORDER BY check_start_time ASC
FORMAT PrettyCompactMonoBlock

Query id: e9252c81-fe1c-4387-b62c-1a4c69e72715

┌─check_duration_ms─┬─mr──────────────┐
│           9168779 │                 │
│          11118219 │                 │
│           7965240 │ python-requests │
│           8240968 │                 │
│           8505889 │                 │
│           8503848 │                 │
│           8318323 │                 │
│           8066711 │                 │
└───────────────────┴─────────────────┘

under sanitizers stateless tests looks a little bit faster, but no significant difference.

SELECT DISTINCT
    check_duration_ms,
    if(report_url LIKE '%268c155b7d4b15524219ea48cf228bf140cf9cdd%', 'python-requests', '') AS mr
FROM checks
WHERE (check_start_time > today()) AND (pull_request_number = 0) AND (report_url LIKE '%stateless_tests_(memory)%')
ORDER BY check_start_time ASC
FORMAT PrettyCompactMonoBlock

Query id: bc1993e5-413f-4d6a-86e2-66e0c1465efb

┌─check_duration_ms─┬─mr──────────────┐
│           7283462 │                 │
│           7491509 │                 │
│           7040965 │ python-requests │
│           6817031 │                 │
│           6866696 │                 │
│           6504630 │                 │
│           6624146 │                 │
│           6728470 │                 │
└───────────────────┴─────────────────┘

tavplubix · 2021-10-11T18:17:29Z

$ ./clickhouse-test  "01921_concurrent|992|993|1154|1593|1079"
Traceback (most recent call last):
  File "./clickhouse-test", line 38, in <module>
    import pandas
ModuleNotFoundError: No module named 'pandas'`

Should we fallback to using clickhouse-test if pandas or clickhouse_driver are not installed (just like we do it for termcolor and jinja)?

tavplubix · 2021-10-11T18:26:06Z

Another issue:

$ ./clickhouse-test  "01921_concurrent|992|993|1154|1593|1079"
Using queries from 'queries' directory
Using clickhouse-client as client program (expecting split build)
Connecting to ClickHouse server... OK

Running 17 stateless tests (MainProcess).

01921_concurrent_ttl_and_normal_merges_zookeeper_long:                  Unknown setting log_comment. Skipping
[ OK ]
01593_concurrent_alter_mutations_kill_many_replicas_long:               Unknown setting log_comment. Skipping
[ OK ]

https://github.com/mymarilyn/clickhouse-driver/blob/9ce201c0689ab04bf85660da78c640dec8c15355/clickhouse_driver/settings/writer.py#L24

azat · 2021-10-11T21:36:10Z

Should we fallback to using clickhouse-test if pandas or clickhouse_driver are not installed (just like we do it for termcolor and jinja)?

Have a fallback for this code looks error prone and does not worth it to me.

P.S. I also think that fallback for jinja is extra complexity that does not worth it.
It is ok (to me) to have a fallback for termcolor since it is not a major functionality of clickhouse-test, unlike templated tests.

Unknown setting log_comment. Skipping

@tavplubix what version of clickhouse-driver do you have? (It should not produce such warnings after mymarilyn/clickhouse-driver#142)

Actually I've talked with @alesapin and he said his strong objections against using clickhouse-driver for clickhouse-test, so I'm planning to rewrite it to simple python requests (and using HTTP interface).

alexey-milovidov · 2021-10-11T21:45:07Z

Actually I've talked with @alesapin and he said his strong objections against using clickhouse-driver for clickhouse-test, so I'm planning to rewrite it to simple python requests (and using HTTP interface).

+1.

@amosbird

Before ClickHouse#29856 `CREATE DATABASE` overwrites it. Reported-by: @amosbird

tavplubix · 2022-09-01T11:03:01Z

tests/clickhouse-test

-                if args.replicated_database:
-                    drop_database_query += " ON CLUSTER test_cluster_database_replicated"


These lines should not have been removed

robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 7, 2021

nikitamikhaylov added the force tests The label does nothing, NOOP, None, nil label Oct 7, 2021

azat marked this pull request as draft October 7, 2021 21:09

azat added 4 commits October 8, 2021 00:09

docker: add pandas/clickhouse_driver into test images

e07a6f3

Suppress some existed warnings in clickhouse-test (will be fixed sepa…

9dd0fca

…rately)

clickhouse-test: do not use persistent connection for simplicity (due…

e2d6698

… to threads)

azat force-pushed the clickhouse-test-python-client branch from 683dfc7 to e2d6698 Compare October 7, 2021 21:09

alexey-milovidov reviewed Oct 8, 2021

View reviewed changes

tests/clickhouse-test Outdated Show resolved Hide resolved

azat added 2 commits October 9, 2021 01:42

clickhouse-test: fix hung check under stress tests

5d6da02

https://clickhouse-test-reports.s3.yandex.net/29856/e2d6698244d43979b3fe2478dfdcd8dc3a91a0fd/stress_test_(address).html#fail1

clickhouse-test: remove not existing options for pylint

42ca2b4

https://clickhouse-test-reports.s3.yandex.net/29856/e2d6698244d43979b3fe2478dfdcd8dc3a91a0fd/style_check/test_run.txt.out.log

azat marked this pull request as ready for review October 8, 2021 22:43

alexey-milovidov merged commit 268c155 into ClickHouse:master Oct 9, 2021

azat deleted the clickhouse-test-python-client branch October 9, 2021 22:25

azat mentioned this pull request Oct 11, 2021

clickhouse-test: Retry DROP DATABASE in case of concurrent DROP/CREATE TABLE #29949

Closed

azat added a commit to azat/ClickHouse that referenced this pull request Oct 12, 2021

clickhouse-test: fix redirect to stderr

fe90c97

Before ClickHouse#29856 `CREATE DATABASE` overwrites it. Reported-by: @amosbird

tavplubix mentioned this pull request Oct 13, 2021

test for rename atomic hanging #30080

Merged

Algunenano mentioned this pull request Oct 28, 2021

Failed tests are retried without need #30802

Closed

tavplubix mentioned this pull request Sep 1, 2022

"Fix" database names collision in functional tests #40876

Merged

tavplubix reviewed Sep 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite clickhouse-test to use python clickhouse_driver #29856

Rewrite clickhouse-test to use python clickhouse_driver #29856

azat commented Oct 7, 2021

azat commented Oct 7, 2021

nikitamikhaylov commented Oct 7, 2021

mergify bot commented Oct 7, 2021

alexey-milovidov commented Oct 7, 2021

azat commented Oct 7, 2021

amosbird commented Oct 8, 2021

alexey-milovidov commented Oct 8, 2021

azat commented Oct 8, 2021

alexey-milovidov commented Oct 8, 2021 •

edited

amosbird commented Oct 8, 2021

amosbird commented Oct 9, 2021

azat commented Oct 9, 2021

azat commented Oct 9, 2021

azat commented Oct 9, 2021

alexey-milovidov commented Oct 9, 2021

azat commented Oct 9, 2021 •

edited

azat commented Oct 9, 2021

tavplubix commented Oct 11, 2021

tavplubix commented Oct 11, 2021

azat commented Oct 11, 2021 •

edited

alexey-milovidov commented Oct 11, 2021

tavplubix Sep 1, 2022

		if args.replicated_database:
		drop_database_query += " ON CLUSTER test_cluster_database_replicated"

Rewrite clickhouse-test to use python clickhouse_driver #29856

Rewrite clickhouse-test to use python clickhouse_driver #29856

Conversation

azat commented Oct 7, 2021

azat commented Oct 7, 2021

nikitamikhaylov commented Oct 7, 2021

mergify bot commented Oct 7, 2021

alexey-milovidov commented Oct 7, 2021

azat commented Oct 7, 2021

amosbird commented Oct 8, 2021

alexey-milovidov commented Oct 8, 2021

azat commented Oct 8, 2021

alexey-milovidov commented Oct 8, 2021 • edited

amosbird commented Oct 8, 2021

amosbird commented Oct 9, 2021

azat commented Oct 9, 2021

azat commented Oct 9, 2021

azat commented Oct 9, 2021

alexey-milovidov commented Oct 9, 2021

azat commented Oct 9, 2021 • edited

azat commented Oct 9, 2021

tavplubix commented Oct 11, 2021

tavplubix commented Oct 11, 2021

azat commented Oct 11, 2021 • edited

alexey-milovidov commented Oct 11, 2021

tavplubix Sep 1, 2022

Choose a reason for hiding this comment

alexey-milovidov commented Oct 8, 2021 •

edited

azat commented Oct 9, 2021 •

edited

azat commented Oct 11, 2021 •

edited