Add metrics for part number in MergeTree in ClickHouse by weeds085490 · Pull Request #17838 · ClickHouse/ClickHouse

weeds085490 · 2020-12-06T14:00:06Z

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):

Add metrics(Parts, PartsActive, PartsInactive) for part number in MergeTree in clickhouse

Detailed description / Documentation draft:

PartsActive corresponds to the commited parts.
PartsInactive corresponds to the parts which are not in commited status.
Parts corresponds to the total number of parts in clickhouse.

We need to deal with three scenarios to calculate Parts、PartsInactive、PartsInactive.

When the table is loaded, recount according to the parts status.
When the table is dropped, recount according to the parts status.
Handle the transfer of parts state.

details:

sundy-li · 2020-12-06T14:17:58Z

BTW, it's a summary metric, why not use system.parts, it has more detail metrics (per database, table ...).
And it's also a table in memory for statistics, clickhouse exporters (or custom HTTP metrics handler) work good with it.

weeds085490 · 2020-12-07T01:27:19Z

BTW, it's a summary metric, why not use system.parts, it has more detail metrics (per database, table ...).
And it's also a table in memory for statistics, clickhouse exporters (or custom HTTP metrics handler) work good with it.

When we are building the monitoring system of clickhouse, we find the number of parts is an important indicator of the overall situation of a cluster. We recommend adding this information to the default metrics just like PartMutation、ReplicatedFetch.

It is worth mentioning that it is indeed possible to collect monitoring data from system.parts through TCP or HTTP interface. We have two main considerations:

Every time you pull, the number of parts must be recalculated.
Since the growth state of parts is an important indicator of whether the cluster is normal, it should be added to the default metrics.

weeds085490 · 2020-12-07T01:36:57Z

BTW, it's a summary metric, why not use system.parts, it has more detail metrics (per database, table ...).
And it's also a table in memory for statistics, clickhouse exporters (or custom HTTP metrics handler) work good with it.

We can also get information about merge from system.merges, but we also need to add Merge to the default metrics

weeds085490 · 2020-12-17T06:50:53Z

@akuzm I have added functional tests for the code. Under normal circumstances, the number of Parts is the same as the data aggregated by system.parts. If we delete the table in the atomic database, the deletion of the table is lazy. At this time, the data may be temporarily inconsistent. PTAL.

alexey-milovidov · 2020-12-21T01:19:05Z

@weeds085490 The code looks Ok but the tests deadlocked. I don't see the reason.
Could you please merge/rebase with master?

weeds085490 · 2020-12-21T01:30:21Z

@weeds085490 The code looks Ok but the tests deadlocked. I don't see the reason.
Could you please merge/rebase with master?

Thanks for reviewing. I have merged with master. PTAL

weeds085490 · 2020-12-21T03:53:17Z

@weeds085490 The code looks Ok but the tests deadlocked. I don't see the reason.
Could you please merge/rebase with master?

Thanks for reviewing. I have merged with master. PTAL

@alexey-milovidov DROP TABLE test_table in my function test still hung. It passed when test it locally.

weeds085490 · 2020-12-21T05:54:36Z

@weeds085490 The code looks Ok but the tests deadlocked. I don't see the reason.
Could you please merge/rebase with master?

Thanks for reviewing. I have merged with master. PTAL

@alexey-milovidov DROP TABLE test_table in my function test still hung. It passed when test it locally.

alexey-milovidov · 2020-12-21T07:29:13Z

src/Storages/MergeTree/MergeTreeData.cpp

    LOG_TRACE(log, "dropAllData: removing data from memory.");

    DataPartsVector all_parts(data_parts_by_info.begin(), data_parts_by_info.end());
+    DataPartsVector committed_parts = getDataPartsVector({DataPartState::Committed});


Doesn't it involve recursive mutex locking (see a few lines above)?

Doesn't it involve recursive mutex locking (see a few lines above)?

That did cause a deadlock. If I use an Ordinary database, the deadlock will definitely occur when dropping table.

@alexey-milovidov I have fixed it. PTAL.

weeds085490 · 2020-12-23T07:56:08Z

@alexey-milovidov I have modified the code to make the monitored data consistent with the data obtained by the system.parts query.

The failed test: Functional stateless tests (release, wide parts enabled) may not related to my code.
The failed test: Functional stateless tests flaky check (address) may be caused by tempory temporary inconsistency.

It becomes consistent after the first query possibly caused by query_log. PTAL

alexey-milovidov · 2020-12-23T12:08:30Z

@weeds085490
How this temporary inconsistency possible and how it is related to the query_log?

weeds085490 · 2020-12-23T14:23:26Z

@weeds085490
How this temporary inconsistency possible and how it is related to the query_log?

During the SQL execution of clickhouse, the input data is not based on the same snapshot. For the same filter, we may get two different data in the same SQL when other threads are inserting new data. Let‘s look at execution log of the failed test:
Functional stateless tests flaky check (address)

when executing the SQL:

SELECT COUNT(1) FROM (SELECT SUM(IF(metric = 'Parts', value, 0)) AS Parts, SUM(IF(metric = 'PartsActive', value, 0)) AS PartsActive, SUM(IF(metric = 'PartsInactive', value, 0)) AS PartsInactive FROM system.metrics) as a INNER JOIN (SELECT toInt64(SUM(1)) AS Parts, toInt64(SUM(IF(active = 1, 1, 0))) AS PartsActive, toInt64(SUM(IF(active = 0, 1, 0))) AS PartsInactive FROM system.parts ) as b USING (Parts,PartsActive,PartsInactive);

A new part: 202012_23_23_0 is inserted. So the different input stream may get different parts info.

Then I do another test：

Reducing the flush_interval_milliseconds and collect_interval_milliseconds of metric_log.
I can reproduce this short-term inconsistency stably.

If I close metric_log, then this short-term inconsistency will never reproduce.

@alexey-milovidov PTAL.

weeds085490 · 2020-12-28T01:42:02Z

@weeds085490
How this temporary inconsistency possible and how it is related to the query_log?

During the SQL execution of clickhouse, the input data is not based on the same snapshot. For the same filter, we may get two different data in the same SQL when other threads are inserting new data. Let‘s look at execution log of the failed test:
Functional stateless tests flaky check (address)

when executing the SQL:
SELECT COUNT(1) FROM (SELECT SUM(IF(metric = 'Parts', value, 0)) AS Parts, SUM(IF(metric = 'PartsActive', value, 0)) AS PartsActive, SUM(IF(metric = 'PartsInactive', value, 0)) AS PartsInactive FROM system.metrics) as a INNER JOIN (SELECT toInt64(SUM(1)) AS Parts, toInt64(SUM(IF(active = 1, 1, 0))) AS PartsActive, toInt64(SUM(IF(active = 0, 1, 0))) AS PartsInactive FROM system.parts ) as b USING (Parts,PartsActive,PartsInactive);
A new part: 202012_23_23_0 is inserted. So the different input stream may get different parts info.

Then I do another test：

Reducing the flush_interval_milliseconds and collect_interval_milliseconds of metric_log.
I can reproduce this short-term inconsistency stably.

If I close metric_log, then this short-term inconsistency will never reproduce.

@alexey-milovidov PTAL.

@alexey-milovidov Hi, PTAL

alexey-milovidov · 2020-12-31T14:54:53Z

Ok but it means we need to rewrite the test in another way...

weeds085490 · 2021-01-07T01:59:46Z

@alexey-milovidov I have changed the test method. All functional tests have passed. PTAL

alexey-milovidov

LGTM

alexey-milovidov · 2021-01-07T13:40:49Z

Sorry, the test is still flaky, I will revert this PR and we need to resubmit it.

azat · 2021-01-07T13:44:43Z

tests/queries/0_stateless/01600_count_of_parts_metrics.sh

+) as b USING (Parts,PartsActive,PartsInactive)"
+
+verify(){
+for _ in $(seq 1 10)


The test does not passed after merge - https://clickhouse-test-reports.s3.yandex.net/0/f91626e7ff352d87283e7b8dafed94fd6ef38c8d/fast_test.html#fail1

@weeds085490 can you please take a look? Maybe 10 retries is not enough?

alexey-milovidov · 2021-01-07T21:00:09Z

It failed 4 out of 16 times.

weeds085490 · 2021-01-08T01:46:14Z

It failed 4 out of 16 times.

Ok. Let me take a look

alexey-milovidov · 2021-01-11T20:25:08Z

I will try to revive changes in #18955

alexey-milovidov · 2021-01-15T12:36:22Z

@weeds085490 See #19122.

Add metrics for part number in MergeTree

2a6c146

robot-clickhouse added the pr-improvement Pull request with some product improvements label Dec 6, 2020

akuzm added can be tested labels Dec 7, 2020

weeds085490 and others added 4 commits December 16, 2020 22:02

fix bug when drop data

1065dde

add test from count of parts

00a5956

add test from count of parts

632f4de

add reference

0195666

Merge remote-tracking branch 'origin' into dev/add_metrics_for_parts

8e72b6d

alexey-milovidov reviewed Dec 21, 2020

View reviewed changes

alexey-milovidov self-assigned this Dec 21, 2020

weeds085490 and others added 7 commits December 21, 2020 16:30

fix deadlock

f3fef24

remove some test

8689bea

force drop table sync in test

0c5c979

fix consistence

b339b9d

remove code

d6a69ee

Merge branch 'master' into dev/add_metrics_for_parts

81f9623

fix style

e38ecee

weeds085490 added 2 commits January 6, 2021 17:32

Merge remote-tracking branch 'origin' into dev/add_metrics_for_parts

5f5b86b

refine test

b2e4c0e

alexey-milovidov added 2 commits January 7, 2021 06:36

Update 01600_count_of_parts_metrics.sh

65b4008

Update StorageMergeTree.cpp

703e16d

alexey-milovidov approved these changes Jan 7, 2021

View reviewed changes

alexey-milovidov merged commit f91626e into ClickHouse:master Jan 7, 2021

alexey-milovidov mentioned this pull request Jan 7, 2021

Revert "Add metrics for part number in MergeTree in ClickHouse" #18834

Merged

azat reviewed Jan 7, 2021

View reviewed changes

Conversation

weeds085490 commented Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sundy-li commented Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weeds085490 commented Dec 7, 2020

Uh oh!

weeds085490 commented Dec 7, 2020

Uh oh!

weeds085490 commented Dec 17, 2020

Uh oh!

alexey-milovidov commented Dec 21, 2020

Uh oh!

weeds085490 commented Dec 21, 2020

Uh oh!

weeds085490 commented Dec 21, 2020

Uh oh!

weeds085490 commented Dec 21, 2020

Uh oh!

alexey-milovidov Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weeds085490 Dec 21, 2020

Choose a reason for hiding this comment

Uh oh!

weeds085490 commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Dec 23, 2020

Uh oh!

weeds085490 commented Dec 23, 2020

Uh oh!

weeds085490 commented Dec 28, 2020

Uh oh!

alexey-milovidov commented Dec 31, 2020

Uh oh!

weeds085490 commented Jan 7, 2021

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Jan 7, 2021

Uh oh!

azat Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Jan 7, 2021

Uh oh!

weeds085490 commented Jan 8, 2021

Uh oh!

alexey-milovidov commented Jan 11, 2021

Uh oh!

alexey-milovidov commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

weeds085490 commented Dec 6, 2020 •

edited

Loading

sundy-li commented Dec 6, 2020 •

edited

Loading

alexey-milovidov Dec 21, 2020 •

edited

Loading

weeds085490 commented Dec 23, 2020 •

edited

Loading