Skip to content

Fix quota consuming#20106

Merged
vitlibar merged 5 commits intoClickHouse:masterfrom
vitlibar:fix-quota-consuming
Feb 17, 2021
Merged

Fix quota consuming#20106
vitlibar merged 5 commits intoClickHouse:masterfrom
vitlibar:fix-quota-consuming

Conversation

@vitlibar
Copy link
Copy Markdown
Member

@vitlibar vitlibar commented Feb 5, 2021 •
edited by alexey-milovidov
Loading

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category:

  • Improvement

Changelog entry:

  1. SHOW TABLES is now considered as one query in the quota calculations, not two queries.
  2. SYSTEM queries now consume quota.
  3. Fix calculation of interval's end in quota consumption.

This PR fixes #20018

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Feb 5, 2021
@robot-clickhouse robot-clickhouse added pr-improvement Pull request with some product improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Feb 5, 2021
@vitlibar vitlibar force-pushed the fix-quota-consuming branch from 917f92b to 93710a0 Compare February 5, 2021 19:40
@nikitamikhaylov nikitamikhaylov self-assigned this Feb 8, 2021
nikitamikhaylov
nikitamikhaylov reviewed Feb 9, 2021
View reviewed changes
src/Access/EnabledQuota.cpp Outdated
Copy link
Copy Markdown
Member

@nikitamikhaylov nikitamikhaylov Feb 9, 2021

Choose a reason for hiding this comment

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

I don't understand what do you want to do in this line. / and * have the same priority.

Copy link
Copy Markdown
Member Author

@vitlibar vitlibar Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

/ and then * is a way to remove the remainder. I've added more comments here.

nikitamikhaylov
nikitamikhaylov reviewed Feb 9, 2021
View reviewed changes
src/Access/EnabledQuota.cpp Outdated
Copy link
Copy Markdown
Member

@nikitamikhaylov nikitamikhaylov Feb 9, 2021

Choose a reason for hiding this comment

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

Could you please add some comments about counters_were_reset variable ?

Copy link
Copy Markdown
Member Author

@vitlibar vitlibar Feb 9, 2021

Choose a reason for hiding this comment

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

done

@nikitamikhaylov
Copy link
Copy Markdown
Member

nikitamikhaylov commented Feb 9, 2021

I read the whole file and found a piece of useless code. Sorry, if I am wrong.

@vitlibar
Copy link
Copy Markdown
Member Author

vitlibar commented Feb 9, 2021

I read the whole file and found a piece of useless code. Sorry, if I am wrong.

Yes, it seems to be useless, thank you.

@vitlibar vitlibar force-pushed the fix-quota-consuming branch from b82af65 to 3ff6fee Compare February 9, 2021 15:39
@nikitamikhaylov nikitamikhaylov mentioned this pull request Feb 15, 2021
Closed
@vitlibar vitlibar force-pushed the fix-quota-consuming branch from 44878c2 to c1bca1d Compare February 16, 2021 18:30
@vitlibar vitlibar force-pushed the fix-quota-consuming branch from c1bca1d to 5f8a6ab Compare February 16, 2021 20:55
@vitlibar vitlibar merged commit 753f5b8 into ClickHouse:master Feb 17, 2021
@vitlibar
Copy link
Copy Markdown
Member Author

vitlibar commented Feb 17, 2021 •
edited
Loading

There was a problem unrelated to this PR:
https://clickhouse-test-reports.s3.yandex.net/20106/5f8a6ab9c109a82ab044b6ee573f86320175839a/integration_tests_(asan).html#fail1

Failed to start cluster: 
Command '['docker-compose', '--project-name', 'roottestgrpcprotocolssl', '--file', '/compose/docker_compose_net.yml', '--file', '/ClickHouse/tests/integration/test_grpc_protocol_ssl/_instances/node/docker-compose.yml', 'up', '-d', '--no-recreate']' returned non-zero exit status 1.

#20631

@sunny19930321 sunny19930321 mentioned this pull request Nov 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikitamikhaylov nikitamikhaylov nikitamikhaylov approved these changes

Assignees

@nikitamikhaylov nikitamikhaylov

Labels

pr-improvement Pull request with some product improvements pr-no-backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Show tables command will take 2 quota count.

4 participants

@vitlibar @nikitamikhaylov @alexey-milovidov @robot-clickhouse