Skip to content

YDBD binary args: added database quotas limits #496

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.

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

Merged
merged 7 commits into from
Jan 22, 2024

Conversation

Iliamish
Copy link
Contributor

No description provided.

Copy link

Hi! Thank you for contributing!
The tests on this PR will run after a maintainer adds an ok-to-test label to this PR manually. Thank you for your patience!

@blinkov blinkov changed the title YDBD CLI: Database qoutas limits YDBD CLI: Database quotas limits Dec 11, 2023
@blinkov blinkov added the ok-to-test Special label used to approve a PR for testing on our infrastructure label Dec 11, 2023
@github-actions github-actions bot removed the ok-to-test Special label used to approve a PR for testing on our infrastructure label Dec 11, 2023
@Iliamish Iliamish requested a review from Gazizonoki December 13, 2023 13:08
@Gazizonoki Gazizonoki added the ok-to-test Special label used to approve a PR for testing on our infrastructure label Dec 15, 2023
@github-actions github-actions bot removed the ok-to-test Special label used to approve a PR for testing on our infrastructure label Dec 15, 2023
Copy link

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-release-asan: some tests FAILED for commit 00a7094.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
14029 13891 0 37 93 8

Gazizonoki
Gazizonoki previously approved these changes Dec 15, 2023
@Iliamish
Copy link
Contributor Author

@Gazizonoki Hi! The failed tests do not look like they are related to my request, but because of them i can't merge request. Can you take a look?

@Gazizonoki
Copy link
Collaborator

@Iliamish We still need to discuss whether we need this feature or not. I am not entitled to make this decision alone. We will come back with an answer a bit later.

@asmyasnikov asmyasnikov changed the title YDBD CLI: Database quotas limits YDBD binary args: added database quotas limits Dec 19, 2023
Copy link
Member

@CyberROFL CyberROFL left a comment

Choose a reason for hiding this comment

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

Please get rid of the copy paste in:

  • parser configuration;
  • parsing.

@Iliamish
Copy link
Contributor Author

Please get rid of the copy paste in:

  • parser configuration;
  • parsing.

Good afternoon! I don't quite understand where you can get rid of the copy paste in this code.

@Iliamish Iliamish requested a review from CyberROFL December 25, 2023 08:27
@Iliamish
Copy link
Contributor Author

@CyberROFL Hi! Can you please come back to PR

@Iliamish
Copy link
Contributor Author

@Gazizonoki, Hi! Are there any updates about this PR?

@CyberROFL
Copy link
Member

Move these sections to common place (base class, helpers, etc) and reuse them:

config.Opts->AddLongOption("data-size-hard-quota", "A maximum data size in bytes, new data will be rejected when exceeded").RequiredArgument("NUM").StoreResult(&DataSizeHardQuota);
config.Opts->AddLongOption("data-size-soft-quota", "Data size in bytes (lower than data_size_hard_quota), at this value new data ingestion is re-enabled again").RequiredArgument("NUM").StoreResult(&DataSizeSoftQuota);
if (config.ParseResult->Has("data-size-hard-quota"))
    GRpcRequest.mutable_database_quotas()->set_data_size_hard_quota(DataSizeHardQuota);
if (config.ParseResult->Has("data-size-soft-quota"))
    GRpcRequest.mutable_database_quotas()->set_data_size_soft_quota(DataSizeSoftQuota);

@Iliamish
Copy link
Contributor Author

@CyberROFL Hi! Can you please review updates

@starlinskiy starlinskiy mentioned this pull request Feb 12, 2024
@vitstn vitstn mentioned this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants