Skip to content

Fix 01281_group_by_limit_memory_tracking flackiness#11119

Merged
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
azat:flacky-test-group_by_limit_memory_tracking
May 27, 2020
Merged

Fix 01281_group_by_limit_memory_tracking flackiness#11119
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
azat:flacky-test-group_by_limit_memory_tracking

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented May 21, 2020 •
edited
Loading

Changelog category (leave one):

  • Non-significant (changelog entry is not required)

Cc: @akuzm
Follow-up for: #11022

@blinkov blinkov added the pr-not-for-changelog This PR should not be mentioned in the changelog label May 21, 2020
@azat azat marked this pull request as draft May 21, 2020 21:51
@akuzm
Copy link
Copy Markdown
Contributor

akuzm commented May 22, 2020 •
edited
Loading

To sum up our discussion on TG, the main problems with the test are:

  • Requires exclusive access to the entire server -- can't run it in parallel with other tests, as is often done during development.
  • Uses summation over memory profiler data -- I'm not sure it can work correctly at all. The memory profiler performs some not-so-deterministic sampling and is not intended for precise measurements.
  • It's complicated -- after looking at the test for some time, I couldn't understand the precise condition that it is checking, and had to ask.

This PR fixes none of the above, but tweaks the test to stop failing for now -- well, this is also useful, no objections.

A fix for the above issues could be to use query log data instead of memory profiler, and maybe add a metric for "how much memory was still allocated when the query ended", but we're not sure this would work. Or maybe we don't have to test for it at all, if it's complicated.

By the way, to use query log data, we don't even have to assign a custom query id -- we can just add something unique to the query text, and filter by it.

@azat azat mentioned this pull request May 22, 2020
Merged
@azat
Copy link
Copy Markdown
Member Author

azat commented May 22, 2020

This PR fixes none of the above, but tweaks the test to stop failing for now -- well, this is also useful, no objections.

This was an attempt to make test green ASAP, but once I saw that it does not fix "anything", I've marked it as draft.

A fix for the above issues could be to use query log data instead of memory profiler

Right now query_log cannot be used for this, since there is no information about deallocated memory, and it can be too late to write it to query_log right now (will verify this and see what can be done).

alexey-milovidov added a commit that referenced this pull request May 22, 2020
@alexey-milovidov
Remove unreliable test #11119
a2fabd7
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented May 22, 2020

I'm Ok if we will just dismiss this test (already done in master).

@alexey-milovidov alexey-milovidov self-assigned this May 23, 2020
@azat azat force-pushed the flacky-test-group_by_limit_memory_tracking branch from 22be06b to aa5247f Compare May 23, 2020 19:58
@azat azat marked this pull request as ready for review May 23, 2020 19:58
@azat
Copy link
Copy Markdown
Member Author

azat commented May 27, 2020

@akuzm @alexey-milovidov can you take a look?

@alexey-milovidov alexey-milovidov merged commit 2da6a39 into ClickHouse:master May 27, 2020
@alexey-milovidov alexey-milovidov added testing Special issue with list of bugs found by CI and removed st-waiting-for-fix labels May 27, 2020
@azat azat deleted the flacky-test-group_by_limit_memory_tracking branch May 31, 2020 18:12
azat
azat commented Jun 2, 2020
View reviewed changes
tests/queries/0_stateless/01281_group_by_limit_memory_tracking.sh
function execute_group_by()
{
local opts=(
--max_memory_usage_for_user=$((150<<20))
Copy link
Copy Markdown
Member Author

@azat azat Jun 2, 2020

Choose a reason for hiding this comment

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

Peak memory usage for the main query (with GROUP BY) is ~100MiB (with max_threads=2 as here).
So set max_memory_usage_for_user to 150MiB and if the memory tracking accounting will be incorrect then the second query will fail

Note that we also need one running query for the user (sleep(3)), since max_memory_usage_for_user is installed to 0 once there are no more queries for user.

azat added a commit to azat/ClickHouse that referenced this pull request Sep 10, 2022
@azat
tests: add missing bits into 01281_group_by_limit_memory_tracking
9d485ac 
While rewriting the test in ClickHouse#11119 it looses LIMIT, which makes the test
useless, see details in ClickHouse#11022

Signed-off-by: Azat Khuzhin <[email protected]>
@azat azat mentioned this pull request Sep 10, 2022
Merged
nickitat pushed a commit that referenced this pull request Sep 11, 2022
@azat
tests: fix 01281_group_by_limit_memory_tracking flakiness (#41178)
13f7a82 
* tests: fix 01281_group_by_limit_memory_tracking flakiness

CI: https://s3.amazonaws.com/clickhouse-test-reports/41092/14d8f297d73f9f813c447474310fbfa0c5b18a0f/stateless_tests__debug__[2/3].html
Signed-off-by: Azat Khuzhin <[email protected]>

* tests: add missing bits into 01281_group_by_limit_memory_tracking

While rewriting the test in #11119 it looses LIMIT, which makes the test
useless, see details in #11022

Signed-off-by: Azat Khuzhin <[email protected]>

Signed-off-by: Azat Khuzhin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@alexey-milovidov alexey-milovidov

Labels

no-docs-needed pr-not-for-changelog This PR should not be mentioned in the changelog testing Special issue with list of bugs found by CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@azat @akuzm @alexey-milovidov @blinkov @qoega